[PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Paul Walmsley]

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
<wesley@sifive.com>:
https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60

This version incorporates several changes requested by Stephen
Boyd <sboyd@kernel.org>.

Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Wesley Terpstra <wesley@sifive.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Megan Wachs <megan@sifive.com>
Cc: linux-clk@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 <paul.walmsley@sifive.com>
+S:	Supported
+F:	drivers/clk/analogbits/*
+F:	include/linux/clk/analogbits*
+
 ANDES ARCHITECTURE
 M:	Greentime Hu <green.hu@gmail.com>
 M:	Vincent Chen <deanbo422@gmail.com>
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 000000000000..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 000000000000..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 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;
+
+	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)
+{
+	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)
+ */
+static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)
+{
+	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;
+
+	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);
+
+	/* Round up */
+	c->init_r = div_u64(parent_rate + MAX_POST_DIVR_FREQ - 1,
+			    MAX_POST_DIVR_FREQ);
+
+	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
+ * 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,
+					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;
+	}
+
+	/* 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;
+
+	/* Calculate the Q shift and target VCO rate */
+	divq = __wrpll_calc_divq(target_rate, &target_vco_rate);
+	if (divq == 0)
+		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? */
+		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);
+
+	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,
+						unsigned long parent_rate)
+{
+	u8 fbdiv;
+	u64 n;
+
+	WARN(c->flags & WRPLL_FLAGS_EXT_FEEDBACK_MASK,
+	     "external feedback mode not yet supported");
+
+	fbdiv = __wrpll_calc_fbdiv(c);
+	n = parent_rate * fbdiv * (c->divf + 1);
+	n = div_u64(n, (c->divr + 1));
+	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)
+{
+	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.
+ */
+
+#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
+ * 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)
+
+/**
+ * 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.
+ */
+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);
+
+#endif /* __LINUX_CLK_ANALOGBITS_WRPLL_CLN28HPC_H */
-- 
2.20.1

[PATCH v3 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver

[Paul Walmsley]

Add DT binding documentation for the Linux driver for the SiFive
PRCI clock & reset control IP block, as found on the SiFive
FU540 chip.

This version includes changes requested by Stephen Boyd
<sboyd@kernel.org> and Rob Herring <robh@kernel.org>, and
fixes some errors in the initial version.

Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Megan Wachs <megan@sifive.com>
Cc: linux-clk@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 .../bindings/clock/sifive/fu540-prci.txt      | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt

diff --git a/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
new file mode 100644
index 000000000000..349808f4fb8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
@@ -0,0 +1,46 @@
+SiFive FU540 PRCI bindings
+
+On the FU540 family of SoCs, most system-wide clock and reset integration
+is via the PRCI IP block.
+
+Required properties:
+- compatible: Should be "sifive,<chip>-prci".  Only one value is
+	supported: "sifive,fu540-c000-prci"
+- reg: Should describe the PRCI's register target physical address region
+- clocks: Should point to the hfclk device tree node and the rtcclk
+          device tree node.  The RTC clock here is not a time-of-day clock,
+	  but is instead a high-stability clock source for system timers
+	  and cycle counters.
+- #clock-cells: Should be <1>
+
+The clock consumer should specify the desired clock via the clock ID
+macros defined in include/dt-bindings/clock/sifive-fu540-prci.h.
+These macros begin with PRCI_CLK_.
+
+The hfclk and rtcclk nodes are required, and represent physical
+crystals or resonators located on the PCB.  These nodes should be present
+underneath /, rather than /soc.
+
+Examples:
+
+/* under /, in PCB-specific DT data */
+hfclk: hfclk {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <33333333>;
+	clock-output-names = "hfclk";
+};
+rtcclk: rtcclk {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <1000000>;
+	clock-output-names = "rtcclk";
+};
+
+/* under /soc, in SoC-specific DT data */
+prci: clock-controller@10000000 {
+	compatible = "sifive,fu540-c000-prci";
+	reg = <0x0 0x10000000 0x0 0x1000>;
+	clocks = <&hfclk>, <&rtcclk>;
+	#clock-cells = <1>;
+};
-- 
2.20.1

[PATCH v3 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Paul Walmsley]

Add driver code for the SiFive FU540 PRCI IP block.  This IP block
handles reset and clock control for the SiFive FU540 device and
implements SoC-level clock tree controls and dividers.

Based on code written by Wesley Terpstra <wesley@sifive.com>:
https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60

Boot and PLL rate change were tested on a SiFive HiFive Unleashed
board.

This version includes several changes requested by Stephen Boyd
<sboyd@kernel.org>.

Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Wesley W. Terpstra <wesley@sifive.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Megan Wachs <megan@sifive.com>
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/Kconfig             |   1 +
 drivers/clk/Makefile            |   1 +
 drivers/clk/sifive/Kconfig      |  18 +
 drivers/clk/sifive/Makefile     |   1 +
 drivers/clk/sifive/fu540-prci.c | 630 ++++++++++++++++++++++++++++++++
 5 files changed, 651 insertions(+)
 create mode 100644 drivers/clk/sifive/Kconfig
 create mode 100644 drivers/clk/sifive/Makefile
 create mode 100644 drivers/clk/sifive/fu540-prci.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 83dc90bd5179..d2f94ff9b0fc 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -310,6 +310,7 @@ source "drivers/clk/mvebu/Kconfig"
 source "drivers/clk/qcom/Kconfig"
 source "drivers/clk/renesas/Kconfig"
 source "drivers/clk/samsung/Kconfig"
+source "drivers/clk/sifive/Kconfig"
 source "drivers/clk/sprd/Kconfig"
 source "drivers/clk/sunxi-ng/Kconfig"
 source "drivers/clk/tegra/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 091ee1d8af65..5173a5ae1069 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
 obj-y					+= renesas/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
+obj-y					+= sifive/
 obj-$(CONFIG_ARCH_SIRF)			+= sirf/
 obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
 obj-$(CONFIG_PLAT_SPEAR)		+= spear/
diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
new file mode 100644
index 000000000000..8db4a3eb4782
--- /dev/null
+++ b/drivers/clk/sifive/Kconfig
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menuconfig CLK_SIFIVE
+	bool "SiFive SoC driver support"
+	help
+	  SoC drivers for SiFive Linux-capable SoCs.
+
+if CLK_SIFIVE
+
+config CLK_SIFIVE_FU540_PRCI
+	bool "PRCI driver for SiFive FU540 SoCs"
+	select CLK_ANALOGBITS_WRPLL_CLN28HPC
+	help
+	  Supports the Power Reset Clock interface (PRCI) IP block found in
+	  FU540 SoCs.  If this kernel is meant to run on a SiFive FU540 SoC,
+	  enable this driver.
+
+endif
diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
new file mode 100644
index 000000000000..74d58a4c0756
--- /dev/null
+++ b/drivers/clk/sifive/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)	+= fu540-prci.o
diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
new file mode 100644
index 000000000000..ecf1dfbcc645
--- /dev/null
+++ b/drivers/clk/sifive/fu540-prci.c
@@ -0,0 +1,630 @@
+// 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.
+ *
+ * The FU540 PRCI implements clock and reset control for the SiFive
+ * FU540-C000 chip.  This driver assumes that it has sole control
+ * over all PRCI resources.
+ *
+ * This driver is based on the PRCI driver written by Wesley Terpstra:
+ * https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60
+ *
+ * References:
+ * - SiFive FU540-C000 manual v1p0, Chapter 7 "Clocking and Reset"
+ */
+
+#include <dt-bindings/clock/sifive-fu540-prci.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/clk/analogbits-wrpll-cln28hpc.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/*
+ * EXPECTED_CLK_PARENT_COUNT: how many parent clocks this driver expects:
+ *     hfclk and rtcclk
+ */
+#define EXPECTED_CLK_PARENT_COUNT		2
+
+/*
+ * Register offsets and bitmasks
+ */
+
+/* COREPLLCFG0 */
+#define PRCI_COREPLLCFG0_OFFSET			0x4
+# define PRCI_COREPLLCFG0_DIVR_SHIFT		0
+# define PRCI_COREPLLCFG0_DIVR_MASK		(0x3f << PRCI_COREPLLCFG0_DIVR_SHIFT)
+# define PRCI_COREPLLCFG0_DIVF_SHIFT		6
+# define PRCI_COREPLLCFG0_DIVF_MASK		(0x1ff << PRCI_COREPLLCFG0_DIVF_SHIFT)
+# define PRCI_COREPLLCFG0_DIVQ_SHIFT		15
+# define PRCI_COREPLLCFG0_DIVQ_MASK		(0x7 << PRCI_COREPLLCFG0_DIVQ_SHIFT)
+# define PRCI_COREPLLCFG0_RANGE_SHIFT		18
+# define PRCI_COREPLLCFG0_RANGE_MASK		(0x7 << PRCI_COREPLLCFG0_RANGE_SHIFT)
+# define PRCI_COREPLLCFG0_BYPASS_SHIFT		24
+# define PRCI_COREPLLCFG0_BYPASS_MASK		(0x1 << PRCI_COREPLLCFG0_BYPASS_SHIFT)
+# define PRCI_COREPLLCFG0_FSE_SHIFT		25
+# define PRCI_COREPLLCFG0_FSE_MASK		(0x1 << PRCI_COREPLLCFG0_FSE_SHIFT)
+# define PRCI_COREPLLCFG0_LOCK_SHIFT		31
+# define PRCI_COREPLLCFG0_LOCK_MASK		(0x1 << PRCI_COREPLLCFG0_LOCK_SHIFT)
+
+/* DDRPLLCFG0 */
+#define PRCI_DDRPLLCFG0_OFFSET			0xc
+# define PRCI_DDRPLLCFG0_DIVR_SHIFT		0
+# define PRCI_DDRPLLCFG0_DIVR_MASK		(0x3f << PRCI_DDRPLLCFG0_DIVR_SHIFT)
+# define PRCI_DDRPLLCFG0_DIVF_SHIFT		6
+# define PRCI_DDRPLLCFG0_DIVF_MASK		(0x1ff << PRCI_DDRPLLCFG0_DIVF_SHIFT)
+# define PRCI_DDRPLLCFG0_DIVQ_SHIFT		15
+# define PRCI_DDRPLLCFG0_DIVQ_MASK		(0x7 << PRCI_DDRPLLCFG0_DIVQ_SHIFT)
+# define PRCI_DDRPLLCFG0_RANGE_SHIFT		18
+# define PRCI_DDRPLLCFG0_RANGE_MASK		(0x7 << PRCI_DDRPLLCFG0_RANGE_SHIFT)
+# define PRCI_DDRPLLCFG0_BYPASS_SHIFT		24
+# define PRCI_DDRPLLCFG0_BYPASS_MASK		(0x1 << PRCI_DDRPLLCFG0_BYPASS_SHIFT)
+# define PRCI_DDRPLLCFG0_FSE_SHIFT		25
+# define PRCI_DDRPLLCFG0_FSE_MASK		(0x1 << PRCI_DDRPLLCFG0_FSE_SHIFT)
+# define PRCI_DDRPLLCFG0_LOCK_SHIFT		31
+# define PRCI_DDRPLLCFG0_LOCK_MASK		(0x1 << PRCI_DDRPLLCFG0_LOCK_SHIFT)
+
+/* DDRPLLCFG1 */
+#define PRCI_DDRPLLCFG1_OFFSET			0x10
+# define PRCI_DDRPLLCFG1_CKE_SHIFT		24
+# define PRCI_DDRPLLCFG1_CKE_MASK		(0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
+
+/* GEMGXLPLLCFG0 */
+#define PRCI_GEMGXLPLLCFG0_OFFSET		0x1c
+# define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT		0
+# define PRCI_GEMGXLPLLCFG0_DIVR_MASK		(0x3f << PRCI_GEMGXLPLLCFG0_DIVR_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_DIVF_SHIFT		6
+# define PRCI_GEMGXLPLLCFG0_DIVF_MASK		(0x1ff << PRCI_GEMGXLPLLCFG0_DIVF_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT		15
+# define PRCI_GEMGXLPLLCFG0_DIVQ_MASK		(0x7 << PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_RANGE_SHIFT		18
+# define PRCI_GEMGXLPLLCFG0_RANGE_MASK		(0x7 << PRCI_GEMGXLPLLCFG0_RANGE_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT	24
+# define PRCI_GEMGXLPLLCFG0_BYPASS_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_FSE_SHIFT		25
+# define PRCI_GEMGXLPLLCFG0_FSE_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_FSE_SHIFT)
+# define PRCI_GEMGXLPLLCFG0_LOCK_SHIFT		31
+# define PRCI_GEMGXLPLLCFG0_LOCK_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_LOCK_SHIFT)
+
+/* GEMGXLPLLCFG1 */
+#define PRCI_GEMGXLPLLCFG1_OFFSET		0x20
+# define PRCI_GEMGXLPLLCFG1_CKE_SHIFT		24
+# define PRCI_GEMGXLPLLCFG1_CKE_MASK		(0x1 << PRCI_GEMGXLPLLCFG1_CKE_SHIFT)
+
+/* CORECLKSEL */
+#define PRCI_CORECLKSEL_OFFSET			0x24
+# define PRCI_CORECLKSEL_CORECLKSEL_SHIFT	0
+# define PRCI_CORECLKSEL_CORECLKSEL_MASK	(0x1 << PRCI_CORECLKSEL_CORECLKSEL_SHIFT)
+
+/* DEVICESRESETREG */
+#define PRCI_DEVICESRESETREG_OFFSET			0x28
+# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT	0
+# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT)
+# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT	1
+# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT)
+# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT	2
+# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT)
+# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT	3
+# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT)
+# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT	5
+# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_MASK		(0x1 << PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT)
+
+/* CLKMUXSTATUSREG */
+#define PRCI_CLKMUXSTATUSREG_OFFSET			0x2c
+# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT	1
+# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK	(0x1 << PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT)
+
+/*
+ * Private structures
+ */
+
+/**
+ * struct __prci_data - per-device-instance data
+ * @va: base virtual address of the PRCI IP block
+ * @hw_clks: encapsulates struct clk_hw records
+ *
+ * PRCI per-device instance data
+ */
+struct __prci_data {
+	void __iomem *va;
+	struct clk_hw_onecell_data hw_clks;
+};
+
+/**
+ * struct __prci_wrpll_data - WRPLL configuration and integration data
+ * @c: WRPLL current configuration record
+ * @enable_bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
+ * @disable_bypass: fn ptr to code to not bypass the WRPLL (or NULL)
+ * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
+ *
+ * @enable_bypass and @disable_bypass are used for WRPLL instances
+ * that contain a separate external glitchless clock mux downstream
+ * from the PLL.  The WRPLL internal bypass mux is not glitchless.
+ */
+struct __prci_wrpll_data {
+	struct analogbits_wrpll_cfg c;
+	void (*enable_bypass)(struct __prci_data *pd);
+	void (*disable_bypass)(struct __prci_data *pd);
+	u8 cfg0_offs;
+};
+
+/**
+ * struct __prci_clock - describes a clock device managed by PRCI
+ * @name: user-readable clock name string - should match the manual
+ * @parent_name: parent name for this clock
+ * @ops: struct clk_ops for the Linux clock framework to use for control
+ * @hw: Linux-private clock data
+ * @pwd: WRPLL-specific data, associated with this clock (if not NULL)
+ * @pd: PRCI-specific data associated with this clock (if not NULL)
+ *
+ * PRCI clock data.  Used by the PRCI driver to register PRCI-provided
+ * clocks to the Linux clock infrastructure.
+ */
+struct __prci_clock {
+	const char *name;
+	const char *parent_name;
+	const struct clk_ops *ops;
+	struct clk_hw hw;
+	struct __prci_wrpll_data *pwd;
+	struct __prci_data *pd;
+};
+
+#define clk_hw_to_prci_clock(pwd) container_of(pwd, struct __prci_clock, hw)
+
+/*
+ * Private functions
+ */
+
+/**
+ * __prci_readl() - read from a PRCI register
+ * @pd: PRCI context
+ * @offs: register offset to read from (in bytes, from PRCI base address)
+ *
+ * Read the register located at offset @offs from the base virtual
+ * address of the PRCI register target described by @pd, and return
+ * the value to the caller.
+ *
+ * Context: Any context.
+ *
+ * Return: the contents of the register described by @pd and @offs.
+ */
+static u32 __prci_readl(struct __prci_data *pd, u32 offs)
+{
+	return readl_relaxed(pd->va + offs);
+}
+
+static void __prci_writel(u32 v, u32 offs, struct __prci_data *pd)
+{
+	return writel_relaxed(v, pd->va + offs);
+}
+
+/* WRPLL-related private functions */
+
+/**
+ * __prci_wrpll_unpack() - unpack WRPLL configuration registers into parameters
+ * @c: ptr to a struct analogbits_wrpll_cfg record to write config into
+ * @r: value read from the PRCI PLL configuration register
+ *
+ * Given a value @r read from an FU540 PRCI PLL configuration register,
+ * split it into fields and populate it into the WRPLL configuration record
+ * pointed to by @c.
+ *
+ * The COREPLLCFG0 macros are used below, but the other *PLLCFG0 macros
+ * have the same register layout.
+ *
+ * Context: Any context.
+ */
+static void __prci_wrpll_unpack(struct analogbits_wrpll_cfg *c, u32 r)
+{
+	u32 v;
+
+	v = r & PRCI_COREPLLCFG0_DIVR_MASK;
+	v >>= PRCI_COREPLLCFG0_DIVR_SHIFT;
+	c->divr = v;
+
+	v = r & PRCI_COREPLLCFG0_DIVF_MASK;
+	v >>= PRCI_COREPLLCFG0_DIVF_SHIFT;
+	c->divf = v;
+
+	v = r & PRCI_COREPLLCFG0_DIVQ_MASK;
+	v >>= PRCI_COREPLLCFG0_DIVQ_SHIFT;
+	c->divq = v;
+
+	v = r & PRCI_COREPLLCFG0_RANGE_MASK;
+	v >>= PRCI_COREPLLCFG0_RANGE_SHIFT;
+	c->range = v;
+
+	c->flags &= (WRPLL_FLAGS_INT_FEEDBACK_MASK |
+		     WRPLL_FLAGS_EXT_FEEDBACK_MASK);
+
+	/* external feedback mode not supported */
+	c->flags |= WRPLL_FLAGS_INT_FEEDBACK_MASK;
+}
+
+/**
+ * __prci_wrpll_pack() - pack PLL configuration parameters into a register value
+ * @c: pointer to a struct analogbits_wrpll_cfg record containing the PLL's cfg
+ *
+ * Using a set of WRPLL configuration values pointed to by @c,
+ * assemble a PRCI PLL configuration register value, and return it to
+ * the caller.
+ *
+ * Context: Any context.  Caller must ensure that the contents of the
+ *          record pointed to by @c do not change during the execution
+ *          of this function.
+ *
+ * Returns: a value suitable for writing into a PRCI PLL configuration
+ *          register
+ */
+static u32 __prci_wrpll_pack(struct analogbits_wrpll_cfg * const c)
+{
+	u32 r = 0;
+
+	r |= c->divr << PRCI_COREPLLCFG0_DIVR_SHIFT;
+	r |= c->divf << PRCI_COREPLLCFG0_DIVF_SHIFT;
+	r |= c->divq << PRCI_COREPLLCFG0_DIVQ_SHIFT;
+	r |= c->range << PRCI_COREPLLCFG0_RANGE_SHIFT;
+
+	/* external feedback mode not supported */
+	r |= PRCI_COREPLLCFG0_FSE_MASK;
+
+	return r;
+}
+
+/**
+ * __prci_wrpll_read_cfg() - read the WRPLL configuration from the PRCI
+ * @pd: PRCI context
+ * @pwd: PRCI WRPLL metadata
+ *
+ * Read the current configuration of the PLL identified by @pwd from
+ * the PRCI identified by @pd, and store it into the local configuration
+ * cache in @pwd.
+ *
+ * Context: Any context.  Caller must prevent the records pointed to by
+ *          @pd and @pwd from changing during execution.
+ */
+static void __prci_wrpll_read_cfg(struct __prci_data *pd,
+				  struct __prci_wrpll_data *pwd)
+{
+	__prci_wrpll_unpack(&pwd->c, __prci_readl(pd, pwd->cfg0_offs));
+}
+
+/**
+ * __prci_wrpll_write_cfg() - write WRPLL configuration into the PRCI
+ * @pd: PRCI context
+ * @pwd: PRCI WRPLL metadata
+ * @c: WRPLL configuration record to write
+ *
+ * Write the WRPLL configuration described by @c into the WRPLL
+ * configuration register identified by @pwd in the PRCI instance
+ * described by @c.  Make a cached copy of the WRPLL's current
+ * configuration so it can be used by other code.
+ *
+ * Context: Any context.  Caller must prevent the records pointed to by
+ *          @pd and @pwd from changing during execution.
+ */
+static void __prci_wrpll_write_cfg(struct __prci_data *pd,
+				   struct __prci_wrpll_data *pwd,
+				   struct analogbits_wrpll_cfg *c)
+{
+	__prci_writel(__prci_wrpll_pack(c), pwd->cfg0_offs, pd);
+
+	memcpy(&pwd->c, c, sizeof(struct analogbits_wrpll_cfg));
+}
+
+/* Core clock mux control */
+
+/**
+ * __prci_coreclksel_use_hfclk() - switch the CORECLK mux to output HFCLK
+ * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
+ *
+ * Switch the CORECLK mux to the HFCLK input source; return once complete.
+ *
+ * Context: Any context.  Caller must prevent concurrent changes to the
+ *          PRCI_CORECLKSEL_OFFSET register.
+ */
+static void __prci_coreclksel_use_hfclk(struct __prci_data *pd)
+{
+	u32 r;
+
+	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
+	r |= PRCI_CORECLKSEL_CORECLKSEL_MASK;
+	__prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
+
+	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
+}
+
+/**
+ * __prci_coreclksel_use_corepll() - switch the CORECLK mux to output COREPLL
+ * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
+ *
+ * Switch the CORECLK mux to the PLL output clock; return once complete.
+ *
+ * Context: Any context.  Caller must prevent concurrent changes to the
+ *          PRCI_CORECLKSEL_OFFSET register.
+ */
+static void __prci_coreclksel_use_corepll(struct __prci_data *pd)
+{
+	u32 r;
+
+	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
+	r &= ~PRCI_CORECLKSEL_CORECLKSEL_MASK;
+	__prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
+
+	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
+}
+
+/*
+ * Linux clock framework integration
+ *
+ * See the Linux clock framework documentation for more information on
+ * these functions.
+ */
+
+static unsigned long sifive_fu540_prci_wrpll_recalc_rate(struct clk_hw *hw,
+							 unsigned long parent_rate)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_wrpll_data *pwd = pc->pwd;
+
+	return analogbits_wrpll_calc_output_rate(&pwd->c, parent_rate);
+}
+
+static long sifive_fu540_prci_wrpll_round_rate(struct clk_hw *hw,
+					       unsigned long rate,
+					       unsigned long *parent_rate)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_wrpll_data *pwd = pc->pwd;
+	struct analogbits_wrpll_cfg c;
+
+	memcpy(&c, &pwd->c, sizeof(c));
+
+	analogbits_wrpll_configure_for_rate(&c, rate, *parent_rate);
+
+	return analogbits_wrpll_calc_output_rate(&c, *parent_rate);
+}
+
+static int sifive_fu540_prci_wrpll_set_rate(struct clk_hw *hw,
+					    unsigned long rate,
+					    unsigned long parent_rate)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_wrpll_data *pwd = pc->pwd;
+	struct __prci_data *pd = pc->pd;
+	int r;
+
+	r = analogbits_wrpll_configure_for_rate(&pwd->c, rate, parent_rate);
+	if (r)
+		return r;
+
+	if (pwd->enable_bypass)
+		pwd->enable_bypass(pd);
+
+	__prci_wrpll_write_cfg(pd, pwd, &pwd->c);
+
+	udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));
+
+	if (pwd->disable_bypass)
+		pwd->disable_bypass(pd);
+
+	return 0;
+}
+
+static const struct clk_ops sifive_fu540_prci_wrpll_clk_ops = {
+	.set_rate = sifive_fu540_prci_wrpll_set_rate,
+	.round_rate = sifive_fu540_prci_wrpll_round_rate,
+	.recalc_rate = sifive_fu540_prci_wrpll_recalc_rate,
+};
+
+static const struct clk_ops sifive_fu540_prci_wrpll_ro_clk_ops = {
+	.recalc_rate = sifive_fu540_prci_wrpll_recalc_rate,
+};
+
+/* TLCLKSEL clock integration */
+
+static unsigned long sifive_fu540_prci_tlclksel_recalc_rate(struct clk_hw *hw,
+							    unsigned long parent_rate)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 v;
+	u8 div;
+
+	v = __prci_readl(pd, PRCI_CLKMUXSTATUSREG_OFFSET);
+	v &= PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK;
+	div = v ? 1 : 2;
+
+	return div_u64(parent_rate, div);
+}
+
+static const struct clk_ops sifive_fu540_prci_tlclksel_clk_ops = {
+	.recalc_rate = sifive_fu540_prci_tlclksel_recalc_rate,
+};
+
+/*
+ * PRCI integration data for each WRPLL instance
+ */
+
+static struct __prci_wrpll_data __prci_corepll_data = {
+	.cfg0_offs = PRCI_COREPLLCFG0_OFFSET,
+	.enable_bypass = __prci_coreclksel_use_hfclk,
+	.disable_bypass = __prci_coreclksel_use_corepll,
+};
+
+static struct __prci_wrpll_data __prci_ddrpll_data = {
+	.cfg0_offs = PRCI_DDRPLLCFG0_OFFSET,
+};
+
+static struct __prci_wrpll_data __prci_gemgxlpll_data = {
+	.cfg0_offs = PRCI_GEMGXLPLLCFG0_OFFSET,
+};
+
+/*
+ * List of clock controls provided by the PRCI
+ */
+
+static struct __prci_clock __prci_init_clocks[] = {
+	[PRCI_CLK_COREPLL] = {
+		.name = "corepll",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu540_prci_wrpll_clk_ops,
+		.pwd = &__prci_corepll_data,
+	},
+	[PRCI_CLK_DDRPLL] = {
+		.name = "ddrpll",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu540_prci_wrpll_ro_clk_ops,
+		.pwd = &__prci_ddrpll_data,
+	},
+	[PRCI_CLK_GEMGXLPLL] = {
+		.name = "gemgxlpll",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu540_prci_wrpll_clk_ops,
+		.pwd = &__prci_gemgxlpll_data,
+	},
+	[PRCI_CLK_TLCLK] = {
+		.name = "tlclk",
+		.parent_name = "corepll",
+		.ops = &sifive_fu540_prci_tlclksel_clk_ops,
+	},
+};
+
+/**
+ * __prci_register_clocks() - register clock controls in the PRCI with Linux
+ * @dev: Linux struct device *
+ *
+ * Register the list of clock controls described in __prci_init_plls[] with
+ * the Linux clock framework.
+ *
+ * Return: 0 upon success or a negative error code upon failure.
+ */
+static int __prci_register_clocks(struct device *dev, struct __prci_data *pd)
+{
+	struct clk_init_data init;
+	struct __prci_clock *pic;
+	int parent_count, i, clk_hw_count, r;
+
+	parent_count = of_clk_get_parent_count(dev->of_node);
+	if (parent_count != EXPECTED_CLK_PARENT_COUNT) {
+		dev_err(dev, "expected only two parent clocks, found %d\n",
+			parent_count);
+		return -EINVAL;
+	}
+
+	memset(&init, 0, sizeof(struct clk_init_data));
+
+	/* Register PLLs */
+	clk_hw_count = sizeof(__prci_init_clocks) / sizeof(struct __prci_clock);
+
+	for (i = 0; i < clk_hw_count; ++i) {
+		pic = &__prci_init_clocks[i];
+
+		init.name = pic->name;
+		init.parent_names = &pic->parent_name;
+		init.num_parents = 1;
+		init.ops = pic->ops;
+		pic->hw.init = &init;
+
+		pic->pd = pd;
+
+		if (pic->pwd)
+			__prci_wrpll_read_cfg(pd, pic->pwd);
+
+		r = devm_clk_hw_register(dev, &pic->hw);
+		if (r) {
+			dev_warn(dev, "Failed to register clock %s: %d\n",
+				 init.name, r);
+			return r;
+		}
+
+		r = clk_hw_register_clkdev(&pic->hw, pic->name, dev_name(dev));
+		if (r) {
+			dev_warn(dev, "Failed to register clkdev for %s: %d\n",
+				 init.name, r);
+			return r;
+		}
+
+		pd->hw_clks.hws[i] = &pic->hw;
+	}
+
+	pd->hw_clks.num = i;
+
+	r = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					&pd->hw_clks);
+	if (r) {
+		dev_err(dev, "could not add hw_provider: %d\n", r);
+		return r;
+	}
+
+	return 0;
+}
+
+/*
+ * Linux device model integration
+ *
+ * See the Linux device model documentation for more information about
+ * these functions.
+ */
+static int sifive_fu540_prci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct __prci_data *pd;
+	int r;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pd->va = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pd->va))
+		return PTR_ERR(pd->va);
+
+	r = __prci_register_clocks(dev, pd);
+	if (r) {
+		dev_err(dev, "could not register clocks: %d\n", r);
+		return r;
+	}
+
+	dev_dbg(dev, "SiFive FU540 PRCI probed\n");
+
+	return 0;
+}
+
+static const struct of_device_id sifive_fu540_prci_of_match[] = {
+	{ .compatible = "sifive,fu540-c000-prci", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sifive_fu540_prci_of_match);
+
+static struct platform_driver sifive_fu540_prci_driver = {
+	.driver	= {
+		.name = "sifive-fu540-prci",
+		.of_match_table = sifive_fu540_prci_of_match,
+	},
+	.probe = sifive_fu540_prci_probe,
+};
+
+static int __init sifive_fu540_prci_init(void)
+{
+	return platform_driver_register(&sifive_fu540_prci_driver);
+}
+core_initcall(sifive_fu540_prci_init);
-- 
2.20.1

Re: [PATCH v3 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver

[Rob Herring]

On Thu, 11 Apr 2019 01:27:34 -0700, Paul Walmsley wrote:
> Add DT binding documentation for the Linux driver for the SiFive
> PRCI clock & reset control IP block, as found on the SiFive
> FU540 chip.
> 
> This version includes changes requested by Stephen Boyd
> <sboyd@kernel.org> and Rob Herring <robh@kernel.org>, and
> fixes some errors in the initial version.
> 
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Megan Wachs <megan@sifive.com>
> Cc: linux-clk@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  .../bindings/clock/sifive/fu540-prci.txt      | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Stephen Boyd]

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
> <wesley@sifive.com>:
> https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60
> 
> This version incorporates several changes requested by Stephen
> Boyd <sboyd@kernel.org>.
> 
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Wesley Terpstra <wesley@sifive.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Megan Wachs <megan@sifive.com>
> Cc: linux-clk@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

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Paul Walmsley]

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 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Atish Patra]

On 4/11/19 1:28 AM, Paul Walmsley wrote:
> Add driver code for the SiFive FU540 PRCI IP block.  This IP block
> handles reset and clock control for the SiFive FU540 device and
> implements SoC-level clock tree controls and dividers.
> 
> Based on code written by Wesley Terpstra <wesley@sifive.com>:
> https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60
> 
> Boot and PLL rate change were tested on a SiFive HiFive Unleashed
> board.
> 
> This version includes several changes requested by Stephen Boyd
> <sboyd@kernel.org>.
> 
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Wesley W. Terpstra <wesley@sifive.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Megan Wachs <megan@sifive.com>
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> ---
>   drivers/clk/Kconfig             |   1 +
>   drivers/clk/Makefile            |   1 +
>   drivers/clk/sifive/Kconfig      |  18 +
>   drivers/clk/sifive/Makefile     |   1 +
>   drivers/clk/sifive/fu540-prci.c | 630 ++++++++++++++++++++++++++++++++
>   5 files changed, 651 insertions(+)
>   create mode 100644 drivers/clk/sifive/Kconfig
>   create mode 100644 drivers/clk/sifive/Makefile
>   create mode 100644 drivers/clk/sifive/fu540-prci.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 83dc90bd5179..d2f94ff9b0fc 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -310,6 +310,7 @@ source "drivers/clk/mvebu/Kconfig"
>   source "drivers/clk/qcom/Kconfig"
>   source "drivers/clk/renesas/Kconfig"
>   source "drivers/clk/samsung/Kconfig"
> +source "drivers/clk/sifive/Kconfig"
>   source "drivers/clk/sprd/Kconfig"
>   source "drivers/clk/sunxi-ng/Kconfig"
>   source "drivers/clk/tegra/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 091ee1d8af65..5173a5ae1069 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
>   obj-y					+= renesas/
>   obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>   obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
> +obj-y					+= sifive/
>   obj-$(CONFIG_ARCH_SIRF)			+= sirf/
>   obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
>   obj-$(CONFIG_PLAT_SPEAR)		+= spear/
> diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> new file mode 100644
> index 000000000000..8db4a3eb4782
> --- /dev/null
> +++ b/drivers/clk/sifive/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menuconfig CLK_SIFIVE
> +	bool "SiFive SoC driver support"
> +	help
> +	  SoC drivers for SiFive Linux-capable SoCs.
> +
> +if CLK_SIFIVE
> +
> +config CLK_SIFIVE_FU540_PRCI
> +	bool "PRCI driver for SiFive FU540 SoCs"
> +	select CLK_ANALOGBITS_WRPLL_CLN28HPC
> +	help
> +	  Supports the Power Reset Clock interface (PRCI) IP block found in
> +	  FU540 SoCs.  If this kernel is meant to run on a SiFive FU540 SoC,
> +	  enable this driver.
> +
> +endif
> diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
> new file mode 100644
> index 000000000000..74d58a4c0756
> --- /dev/null
> +++ b/drivers/clk/sifive/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)	+= fu540-prci.o
> diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> new file mode 100644
> index 000000000000..ecf1dfbcc645
> --- /dev/null
> +++ b/drivers/clk/sifive/fu540-prci.c
> @@ -0,0 +1,630 @@
> +// 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.
> + *
> + * The FU540 PRCI implements clock and reset control for the SiFive
> + * FU540-C000 chip.  This driver assumes that it has sole control
> + * over all PRCI resources.
> + *
> + * This driver is based on the PRCI driver written by Wesley Terpstra:
> + * https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60
> + *
> + * References:
> + * - SiFive FU540-C000 manual v1p0, Chapter 7 "Clocking and Reset"
> + */
> +
> +#include <dt-bindings/clock/sifive-fu540-prci.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk/analogbits-wrpll-cln28hpc.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/*
> + * EXPECTED_CLK_PARENT_COUNT: how many parent clocks this driver expects:
> + *     hfclk and rtcclk
> + */
> +#define EXPECTED_CLK_PARENT_COUNT		2
> +
> +/*
> + * Register offsets and bitmasks
> + */
> +
> +/* COREPLLCFG0 */
> +#define PRCI_COREPLLCFG0_OFFSET			0x4
> +# define PRCI_COREPLLCFG0_DIVR_SHIFT		0
> +# define PRCI_COREPLLCFG0_DIVR_MASK		(0x3f << PRCI_COREPLLCFG0_DIVR_SHIFT)
> +# define PRCI_COREPLLCFG0_DIVF_SHIFT		6
> +# define PRCI_COREPLLCFG0_DIVF_MASK		(0x1ff << PRCI_COREPLLCFG0_DIVF_SHIFT)
> +# define PRCI_COREPLLCFG0_DIVQ_SHIFT		15
> +# define PRCI_COREPLLCFG0_DIVQ_MASK		(0x7 << PRCI_COREPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_COREPLLCFG0_RANGE_SHIFT		18
> +# define PRCI_COREPLLCFG0_RANGE_MASK		(0x7 << PRCI_COREPLLCFG0_RANGE_SHIFT)
> +# define PRCI_COREPLLCFG0_BYPASS_SHIFT		24
> +# define PRCI_COREPLLCFG0_BYPASS_MASK		(0x1 << PRCI_COREPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_COREPLLCFG0_FSE_SHIFT		25
> +# define PRCI_COREPLLCFG0_FSE_MASK		(0x1 << PRCI_COREPLLCFG0_FSE_SHIFT)
> +# define PRCI_COREPLLCFG0_LOCK_SHIFT		31
> +# define PRCI_COREPLLCFG0_LOCK_MASK		(0x1 << PRCI_COREPLLCFG0_LOCK_SHIFT)
> +
> +/* DDRPLLCFG0 */
> +#define PRCI_DDRPLLCFG0_OFFSET			0xc
> +# define PRCI_DDRPLLCFG0_DIVR_SHIFT		0
> +# define PRCI_DDRPLLCFG0_DIVR_MASK		(0x3f << PRCI_DDRPLLCFG0_DIVR_SHIFT)
> +# define PRCI_DDRPLLCFG0_DIVF_SHIFT		6
> +# define PRCI_DDRPLLCFG0_DIVF_MASK		(0x1ff << PRCI_DDRPLLCFG0_DIVF_SHIFT)
> +# define PRCI_DDRPLLCFG0_DIVQ_SHIFT		15
> +# define PRCI_DDRPLLCFG0_DIVQ_MASK		(0x7 << PRCI_DDRPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_DDRPLLCFG0_RANGE_SHIFT		18
> +# define PRCI_DDRPLLCFG0_RANGE_MASK		(0x7 << PRCI_DDRPLLCFG0_RANGE_SHIFT)
> +# define PRCI_DDRPLLCFG0_BYPASS_SHIFT		24
> +# define PRCI_DDRPLLCFG0_BYPASS_MASK		(0x1 << PRCI_DDRPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_DDRPLLCFG0_FSE_SHIFT		25
> +# define PRCI_DDRPLLCFG0_FSE_MASK		(0x1 << PRCI_DDRPLLCFG0_FSE_SHIFT)
> +# define PRCI_DDRPLLCFG0_LOCK_SHIFT		31
> +# define PRCI_DDRPLLCFG0_LOCK_MASK		(0x1 << PRCI_DDRPLLCFG0_LOCK_SHIFT)
> +
> +/* DDRPLLCFG1 */
> +#define PRCI_DDRPLLCFG1_OFFSET			0x10
> +# define PRCI_DDRPLLCFG1_CKE_SHIFT		24
> +# define PRCI_DDRPLLCFG1_CKE_MASK		(0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
> +
> +/* GEMGXLPLLCFG0 */
> +#define PRCI_GEMGXLPLLCFG0_OFFSET		0x1c
> +# define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT		0
> +# define PRCI_GEMGXLPLLCFG0_DIVR_MASK		(0x3f << PRCI_GEMGXLPLLCFG0_DIVR_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_DIVF_SHIFT		6
> +# define PRCI_GEMGXLPLLCFG0_DIVF_MASK		(0x1ff << PRCI_GEMGXLPLLCFG0_DIVF_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT		15
> +# define PRCI_GEMGXLPLLCFG0_DIVQ_MASK		(0x7 << PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_RANGE_SHIFT		18
> +# define PRCI_GEMGXLPLLCFG0_RANGE_MASK		(0x7 << PRCI_GEMGXLPLLCFG0_RANGE_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT	24
> +# define PRCI_GEMGXLPLLCFG0_BYPASS_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_FSE_SHIFT		25
> +# define PRCI_GEMGXLPLLCFG0_FSE_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_FSE_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_LOCK_SHIFT		31
> +# define PRCI_GEMGXLPLLCFG0_LOCK_MASK		(0x1 << PRCI_GEMGXLPLLCFG0_LOCK_SHIFT)
> +
> +/* GEMGXLPLLCFG1 */
> +#define PRCI_GEMGXLPLLCFG1_OFFSET		0x20
> +# define PRCI_GEMGXLPLLCFG1_CKE_SHIFT		24
> +# define PRCI_GEMGXLPLLCFG1_CKE_MASK		(0x1 << PRCI_GEMGXLPLLCFG1_CKE_SHIFT)
> +
> +/* CORECLKSEL */
> +#define PRCI_CORECLKSEL_OFFSET			0x24
> +# define PRCI_CORECLKSEL_CORECLKSEL_SHIFT	0
> +# define PRCI_CORECLKSEL_CORECLKSEL_MASK	(0x1 << PRCI_CORECLKSEL_CORECLKSEL_SHIFT)
> +
> +/* DEVICESRESETREG */
> +#define PRCI_DEVICESRESETREG_OFFSET			0x28
> +# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT	0
> +# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT	1
> +# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT	2
> +# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT	3
> +# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_MASK	(0x1 << PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT	5
> +# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_MASK		(0x1 << PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT)
> +
> +/* CLKMUXSTATUSREG */
> +#define PRCI_CLKMUXSTATUSREG_OFFSET			0x2c
> +# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT	1
> +# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK	(0x1 << PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT)
> +
> +/*
> + * Private structures
> + */
> +
> +/**
> + * struct __prci_data - per-device-instance data
> + * @va: base virtual address of the PRCI IP block
> + * @hw_clks: encapsulates struct clk_hw records
> + *
> + * PRCI per-device instance data
> + */
> +struct __prci_data {
> +	void __iomem *va;
> +	struct clk_hw_onecell_data hw_clks;
> +};
> +
> +/**
> + * struct __prci_wrpll_data - WRPLL configuration and integration data
> + * @c: WRPLL current configuration record
> + * @enable_bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
> + * @disable_bypass: fn ptr to code to not bypass the WRPLL (or NULL)
> + * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
> + *
> + * @enable_bypass and @disable_bypass are used for WRPLL instances
> + * that contain a separate external glitchless clock mux downstream
> + * from the PLL.  The WRPLL internal bypass mux is not glitchless.
> + */
> +struct __prci_wrpll_data {
> +	struct analogbits_wrpll_cfg c;
> +	void (*enable_bypass)(struct __prci_data *pd);
> +	void (*disable_bypass)(struct __prci_data *pd);
> +	u8 cfg0_offs;
> +};
> +
> +/**
> + * struct __prci_clock - describes a clock device managed by PRCI
> + * @name: user-readable clock name string - should match the manual
> + * @parent_name: parent name for this clock
> + * @ops: struct clk_ops for the Linux clock framework to use for control
> + * @hw: Linux-private clock data
> + * @pwd: WRPLL-specific data, associated with this clock (if not NULL)
> + * @pd: PRCI-specific data associated with this clock (if not NULL)
> + *
> + * PRCI clock data.  Used by the PRCI driver to register PRCI-provided
> + * clocks to the Linux clock infrastructure.
> + */
> +struct __prci_clock {
> +	const char *name;
> +	const char *parent_name;
> +	const struct clk_ops *ops;
> +	struct clk_hw hw;
> +	struct __prci_wrpll_data *pwd;
> +	struct __prci_data *pd;
> +};
> +
> +#define clk_hw_to_prci_clock(pwd) container_of(pwd, struct __prci_clock, hw)
> +
> +/*
> + * Private functions
> + */
> +
> +/**
> + * __prci_readl() - read from a PRCI register
> + * @pd: PRCI context
> + * @offs: register offset to read from (in bytes, from PRCI base address)
> + *
> + * Read the register located at offset @offs from the base virtual
> + * address of the PRCI register target described by @pd, and return
> + * the value to the caller.
> + *
> + * Context: Any context.
> + *
> + * Return: the contents of the register described by @pd and @offs.
> + */
> +static u32 __prci_readl(struct __prci_data *pd, u32 offs)
> +{
> +	return readl_relaxed(pd->va + offs);
> +}
> +
> +static void __prci_writel(u32 v, u32 offs, struct __prci_data *pd)
> +{
> +	return writel_relaxed(v, pd->va + offs);
> +}
> +
> +/* WRPLL-related private functions */
> +
> +/**
> + * __prci_wrpll_unpack() - unpack WRPLL configuration registers into parameters
> + * @c: ptr to a struct analogbits_wrpll_cfg record to write config into
> + * @r: value read from the PRCI PLL configuration register
> + *
> + * Given a value @r read from an FU540 PRCI PLL configuration register,
> + * split it into fields and populate it into the WRPLL configuration record
> + * pointed to by @c.
> + *
> + * The COREPLLCFG0 macros are used below, but the other *PLLCFG0 macros
> + * have the same register layout.
> + *
> + * Context: Any context.
> + */
> +static void __prci_wrpll_unpack(struct analogbits_wrpll_cfg *c, u32 r)
> +{
> +	u32 v;
> +
> +	v = r & PRCI_COREPLLCFG0_DIVR_MASK;
> +	v >>= PRCI_COREPLLCFG0_DIVR_SHIFT;
> +	c->divr = v;
> +
> +	v = r & PRCI_COREPLLCFG0_DIVF_MASK;
> +	v >>= PRCI_COREPLLCFG0_DIVF_SHIFT;
> +	c->divf = v;
> +
> +	v = r & PRCI_COREPLLCFG0_DIVQ_MASK;
> +	v >>= PRCI_COREPLLCFG0_DIVQ_SHIFT;
> +	c->divq = v;
> +
> +	v = r & PRCI_COREPLLCFG0_RANGE_MASK;
> +	v >>= PRCI_COREPLLCFG0_RANGE_SHIFT;
> +	c->range = v;
> +
> +	c->flags &= (WRPLL_FLAGS_INT_FEEDBACK_MASK |
> +		     WRPLL_FLAGS_EXT_FEEDBACK_MASK);
> +
> +	/* external feedback mode not supported */
> +	c->flags |= WRPLL_FLAGS_INT_FEEDBACK_MASK;
> +}
> +
> +/**
> + * __prci_wrpll_pack() - pack PLL configuration parameters into a register value
> + * @c: pointer to a struct analogbits_wrpll_cfg record containing the PLL's cfg
> + *
> + * Using a set of WRPLL configuration values pointed to by @c,
> + * assemble a PRCI PLL configuration register value, and return it to
> + * the caller.
> + *
> + * Context: Any context.  Caller must ensure that the contents of the
> + *          record pointed to by @c do not change during the execution
> + *          of this function.
> + *
> + * Returns: a value suitable for writing into a PRCI PLL configuration
> + *          register
> + */
> +static u32 __prci_wrpll_pack(struct analogbits_wrpll_cfg * const c)
> +{
> +	u32 r = 0;
> +
> +	r |= c->divr << PRCI_COREPLLCFG0_DIVR_SHIFT;
> +	r |= c->divf << PRCI_COREPLLCFG0_DIVF_SHIFT;
> +	r |= c->divq << PRCI_COREPLLCFG0_DIVQ_SHIFT;
> +	r |= c->range << PRCI_COREPLLCFG0_RANGE_SHIFT;
> +
> +	/* external feedback mode not supported */
> +	r |= PRCI_COREPLLCFG0_FSE_MASK;
> +
> +	return r;
> +}
> +
> +/**
> + * __prci_wrpll_read_cfg() - read the WRPLL configuration from the PRCI
> + * @pd: PRCI context
> + * @pwd: PRCI WRPLL metadata
> + *
> + * Read the current configuration of the PLL identified by @pwd from
> + * the PRCI identified by @pd, and store it into the local configuration
> + * cache in @pwd.
> + *
> + * Context: Any context.  Caller must prevent the records pointed to by
> + *          @pd and @pwd from changing during execution.
> + */
> +static void __prci_wrpll_read_cfg(struct __prci_data *pd,
> +				  struct __prci_wrpll_data *pwd)
> +{
> +	__prci_wrpll_unpack(&pwd->c, __prci_readl(pd, pwd->cfg0_offs));
> +}
> +
> +/**
> + * __prci_wrpll_write_cfg() - write WRPLL configuration into the PRCI
> + * @pd: PRCI context
> + * @pwd: PRCI WRPLL metadata
> + * @c: WRPLL configuration record to write
> + *
> + * Write the WRPLL configuration described by @c into the WRPLL
> + * configuration register identified by @pwd in the PRCI instance
> + * described by @c.  Make a cached copy of the WRPLL's current
> + * configuration so it can be used by other code.
> + *
> + * Context: Any context.  Caller must prevent the records pointed to by
> + *          @pd and @pwd from changing during execution.
> + */
> +static void __prci_wrpll_write_cfg(struct __prci_data *pd,
> +				   struct __prci_wrpll_data *pwd,
> +				   struct analogbits_wrpll_cfg *c)
> +{
> +	__prci_writel(__prci_wrpll_pack(c), pwd->cfg0_offs, pd);
> +
> +	memcpy(&pwd->c, c, sizeof(struct analogbits_wrpll_cfg));
> +}
> +
> +/* Core clock mux control */
> +
> +/**
> + * __prci_coreclksel_use_hfclk() - switch the CORECLK mux to output HFCLK
> + * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
> + *
> + * Switch the CORECLK mux to the HFCLK input source; return once complete.
> + *
> + * Context: Any context.  Caller must prevent concurrent changes to the
> + *          PRCI_CORECLKSEL_OFFSET register.
> + */
> +static void __prci_coreclksel_use_hfclk(struct __prci_data *pd)
> +{
> +	u32 r;
> +
> +	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
> +	r |= PRCI_CORECLKSEL_CORECLKSEL_MASK;
> +	__prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
> +
> +	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
> +}
> +
> +/**
> + * __prci_coreclksel_use_corepll() - switch the CORECLK mux to output COREPLL
> + * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
> + *
> + * Switch the CORECLK mux to the PLL output clock; return once complete.
> + *
> + * Context: Any context.  Caller must prevent concurrent changes to the
> + *          PRCI_CORECLKSEL_OFFSET register.
> + */
> +static void __prci_coreclksel_use_corepll(struct __prci_data *pd)
> +{
> +	u32 r;
> +
> +	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
> +	r &= ~PRCI_CORECLKSEL_CORECLKSEL_MASK;
> +	__prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
> +
> +	r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
> +}
> +
> +/*
> + * Linux clock framework integration
> + *
> + * See the Linux clock framework documentation for more information on
> + * these functions.
> + */
> +
> +static unsigned long sifive_fu540_prci_wrpll_recalc_rate(struct clk_hw *hw,
> +							 unsigned long parent_rate)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_wrpll_data *pwd = pc->pwd;
> +
> +	return analogbits_wrpll_calc_output_rate(&pwd->c, parent_rate);
> +}
> +
> +static long sifive_fu540_prci_wrpll_round_rate(struct clk_hw *hw,
> +					       unsigned long rate,
> +					       unsigned long *parent_rate)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_wrpll_data *pwd = pc->pwd;
> +	struct analogbits_wrpll_cfg c;
> +
> +	memcpy(&c, &pwd->c, sizeof(c));
> +
> +	analogbits_wrpll_configure_for_rate(&c, rate, *parent_rate);
> +
> +	return analogbits_wrpll_calc_output_rate(&c, *parent_rate);
> +}
> +
> +static int sifive_fu540_prci_wrpll_set_rate(struct clk_hw *hw,
> +					    unsigned long rate,
> +					    unsigned long parent_rate)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_wrpll_data *pwd = pc->pwd;
> +	struct __prci_data *pd = pc->pd;
> +	int r;
> +
> +	r = analogbits_wrpll_configure_for_rate(&pwd->c, rate, parent_rate);
> +	if (r)
> +		return r;
> +
> +	if (pwd->enable_bypass)
> +		pwd->enable_bypass(pd);
> +
> +	__prci_wrpll_write_cfg(pd, pwd, &pwd->c);
> +
> +	udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));
> +
> +	if (pwd->disable_bypass)
> +		pwd->disable_bypass(pd);
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops sifive_fu540_prci_wrpll_clk_ops = {
> +	.set_rate = sifive_fu540_prci_wrpll_set_rate,
> +	.round_rate = sifive_fu540_prci_wrpll_round_rate,
> +	.recalc_rate = sifive_fu540_prci_wrpll_recalc_rate,
> +};
> +
> +static const struct clk_ops sifive_fu540_prci_wrpll_ro_clk_ops = {
> +	.recalc_rate = sifive_fu540_prci_wrpll_recalc_rate,
> +};
> +
> +/* TLCLKSEL clock integration */
> +
> +static unsigned long sifive_fu540_prci_tlclksel_recalc_rate(struct clk_hw *hw,
> +							    unsigned long parent_rate)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_data *pd = pc->pd;
> +	u32 v;
> +	u8 div;
> +
> +	v = __prci_readl(pd, PRCI_CLKMUXSTATUSREG_OFFSET);
> +	v &= PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK;
> +	div = v ? 1 : 2;
> +
> +	return div_u64(parent_rate, div);
> +}
> +
> +static const struct clk_ops sifive_fu540_prci_tlclksel_clk_ops = {
> +	.recalc_rate = sifive_fu540_prci_tlclksel_recalc_rate,
> +};
> +
> +/*
> + * PRCI integration data for each WRPLL instance
> + */
> +
> +static struct __prci_wrpll_data __prci_corepll_data = {
> +	.cfg0_offs = PRCI_COREPLLCFG0_OFFSET,
> +	.enable_bypass = __prci_coreclksel_use_hfclk,
> +	.disable_bypass = __prci_coreclksel_use_corepll,
> +};
> +
> +static struct __prci_wrpll_data __prci_ddrpll_data = {
> +	.cfg0_offs = PRCI_DDRPLLCFG0_OFFSET,
> +};
> +
> +static struct __prci_wrpll_data __prci_gemgxlpll_data = {
> +	.cfg0_offs = PRCI_GEMGXLPLLCFG0_OFFSET,
> +};
> +
> +/*
> + * List of clock controls provided by the PRCI
> + */
> +
> +static struct __prci_clock __prci_init_clocks[] = {
> +	[PRCI_CLK_COREPLL] = {
> +		.name = "corepll",
> +		.parent_name = "hfclk",
> +		.ops = &sifive_fu540_prci_wrpll_clk_ops,
> +		.pwd = &__prci_corepll_data,
> +	},
> +	[PRCI_CLK_DDRPLL] = {
> +		.name = "ddrpll",
> +		.parent_name = "hfclk",
> +		.ops = &sifive_fu540_prci_wrpll_ro_clk_ops,
> +		.pwd = &__prci_ddrpll_data,
> +	},
> +	[PRCI_CLK_GEMGXLPLL] = {
> +		.name = "gemgxlpll",
> +		.parent_name = "hfclk",
> +		.ops = &sifive_fu540_prci_wrpll_clk_ops,
> +		.pwd = &__prci_gemgxlpll_data,
> +	},
> +	[PRCI_CLK_TLCLK] = {
> +		.name = "tlclk",
> +		.parent_name = "corepll",
> +		.ops = &sifive_fu540_prci_tlclksel_clk_ops,
> +	},
> +};
> +
> +/**
> + * __prci_register_clocks() - register clock controls in the PRCI with Linux
> + * @dev: Linux struct device *
> + *
> + * Register the list of clock controls described in __prci_init_plls[] with
> + * the Linux clock framework.
> + *
> + * Return: 0 upon success or a negative error code upon failure.
> + */
> +static int __prci_register_clocks(struct device *dev, struct __prci_data *pd)
> +{
> +	struct clk_init_data init;
> +	struct __prci_clock *pic;
> +	int parent_count, i, clk_hw_count, r;
> +
> +	parent_count = of_clk_get_parent_count(dev->of_node);
> +	if (parent_count != EXPECTED_CLK_PARENT_COUNT) {
> +		dev_err(dev, "expected only two parent clocks, found %d\n",
> +			parent_count);
> +		return -EINVAL;
> +	}
> +
> +	memset(&init, 0, sizeof(struct clk_init_data));
> +
> +	/* Register PLLs */
> +	clk_hw_count = sizeof(__prci_init_clocks) / sizeof(struct __prci_clock);
> +
> +	for (i = 0; i < clk_hw_count; ++i) {
> +		pic = &__prci_init_clocks[i];
> +
> +		init.name = pic->name;
> +		init.parent_names = &pic->parent_name;
> +		init.num_parents = 1;
> +		init.ops = pic->ops;
> +		pic->hw.init = &init;
> +
> +		pic->pd = pd;
> +
> +		if (pic->pwd)
> +			__prci_wrpll_read_cfg(pd, pic->pwd);
> +
> +		r = devm_clk_hw_register(dev, &pic->hw);
> +		if (r) {
> +			dev_warn(dev, "Failed to register clock %s: %d\n",
> +				 init.name, r);
> +			return r;
> +		}
> +
> +		r = clk_hw_register_clkdev(&pic->hw, pic->name, dev_name(dev));
> +		if (r) {
> +			dev_warn(dev, "Failed to register clkdev for %s: %d\n",
> +				 init.name, r);
> +			return r;
> +		}
> +
> +		pd->hw_clks.hws[i] = &pic->hw;
> +	}
> +
> +	pd->hw_clks.num = i;
> +
> +	r = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					&pd->hw_clks);
> +	if (r) {
> +		dev_err(dev, "could not add hw_provider: %d\n", r);
> +		return r;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Linux device model integration
> + *
> + * See the Linux device model documentation for more information about
> + * these functions.
> + */
> +static int sifive_fu540_prci_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct __prci_data *pd;
> +	int r;
> +
> +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pd->va = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pd->va))
> +		return PTR_ERR(pd->va);
> +
> +	r = __prci_register_clocks(dev, pd);
> +	if (r) {
> +		dev_err(dev, "could not register clocks: %d\n", r);
> +		return r;
> +	}
> +
> +	dev_dbg(dev, "SiFive FU540 PRCI probed\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sifive_fu540_prci_of_match[] = {
> +	{ .compatible = "sifive,fu540-c000-prci", },

All the existing unleashed devices have prci clock compatible string as
"sifive,aloeprci0" or "sifive,ux00prci0". Should it be added to maintain 
backward compatibility?

Even after adding the compatible string (just for my testing purpose), I 
get this while booting.

[    0.104571] sifive-fu540-prci 10000000.prci: expected only two parent 
clocks, found 1
[    0.112460] sifive-fu540-prci 10000000.prci: could not register 
clocks: -22
[    0.119499] sifive-fu540-prci: probe of 10000000.prci failed with 
error -22

Looking at the DT entries, your DT patch has

+		prci: clock-controller@10000000 {
+			compatible = "sifive,fu540-c000-prci";
+			reg = <0x0 0x10000000 0x0 0x1000>;
+			clocks = <&hfclk>, <&rtcclk>;
+			#clock-cells = <1>;
+		};


while current DT from FSBL 
(https://github.com/sifive/freedom-u540-c000-bootloader/blob/master/fsbl/ux00_fsbl.dts)

prci: prci@10000000 {
			compatible = "sifive,aloeprci0", "sifive,ux00prci0";
			reg = <0x0 0x10000000 0x0 0x1000>;
			reg-names = "control";
			clocks = <&refclk>;
			#clock-cells = <1>;
		};

This seems to be the cause of error. It looks like this patch needs a 
complete different DT (your DT patch) than FSBL provides.

This means everybody must upgrade the FSBL to use your DT patch in their 
boards once this driver is merged. Is this okay?

Regards,
Atish
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sifive_fu540_prci_of_match);
> +
> +static struct platform_driver sifive_fu540_prci_driver = {
> +	.driver	= {
> +		.name = "sifive-fu540-prci",
> +		.of_match_table = sifive_fu540_prci_of_match,
> +	},
> +	.probe = sifive_fu540_prci_probe,
> +};
> +
> +static int __init sifive_fu540_prci_init(void)
> +{
> +	return platform_driver_register(&sifive_fu540_prci_driver);
> +}
> +core_initcall(sifive_fu540_prci_init);
> 

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Paul Walmsley]

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

[Stephen Boyd]

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.
 

Re: [PATCH v3 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Stephen Boyd]

Quoting Paul Walmsley (2019-04-11 01:27:36)
> diff --git a/drivers/clk/sifive/Makefile b/drivers/clk/sifive/Makefile
> new file mode 100644
> index 000000000000..74d58a4c0756
> --- /dev/null
> +++ b/drivers/clk/sifive/Makefile
> @@ -0,0 +1 @@

Any SPDX for this file?

> +obj-$(CONFIG_CLK_SIFIVE_FU540_PRCI)    += fu540-prci.o
> diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> new file mode 100644
> index 000000000000..ecf1dfbcc645
> --- /dev/null
> +++ b/drivers/clk/sifive/fu540-prci.c
> @@ -0,0 +1,630 @@
> +// 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.

Please remove above two hunks as they're part of SPDX already.

> + *
> + * The FU540 PRCI implements clock and reset control for the SiFive
> + * FU540-C000 chip.  This driver assumes that it has sole control
> + * over all PRCI resources.
> + *
> + * This driver is based on the PRCI driver written by Wesley Terpstra:
> + * https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60
> + *
> + * References:
> + * - SiFive FU540-C000 manual v1p0, Chapter 7 "Clocking and Reset"
> + */
> +
> +#include <dt-bindings/clock/sifive-fu540-prci.h>

Please put this after linux.

> +#include <linux/clkdev.h>

Do you need clkdev? Isn't this all DT based? If possible, please don't
use clkdev.

> +#include <linux/clk-provider.h>
> +#include <linux/clk/analogbits-wrpll-cln28hpc.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>

Is this used?

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/*
> + * EXPECTED_CLK_PARENT_COUNT: how many parent clocks this driver expects:
> + *     hfclk and rtcclk
> + */
> +#define EXPECTED_CLK_PARENT_COUNT              2
> +
> +/*
> + * Register offsets and bitmasks
> + */
> +
> +/* COREPLLCFG0 */
> +#define PRCI_COREPLLCFG0_OFFSET                        0x4
> +# define PRCI_COREPLLCFG0_DIVR_SHIFT           0
> +# define PRCI_COREPLLCFG0_DIVR_MASK            (0x3f << PRCI_COREPLLCFG0_DIVR_SHIFT)
> +# define PRCI_COREPLLCFG0_DIVF_SHIFT           6
> +# define PRCI_COREPLLCFG0_DIVF_MASK            (0x1ff << PRCI_COREPLLCFG0_DIVF_SHIFT)
> +# define PRCI_COREPLLCFG0_DIVQ_SHIFT           15
> +# define PRCI_COREPLLCFG0_DIVQ_MASK            (0x7 << PRCI_COREPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_COREPLLCFG0_RANGE_SHIFT          18
> +# define PRCI_COREPLLCFG0_RANGE_MASK           (0x7 << PRCI_COREPLLCFG0_RANGE_SHIFT)
> +# define PRCI_COREPLLCFG0_BYPASS_SHIFT         24
> +# define PRCI_COREPLLCFG0_BYPASS_MASK          (0x1 << PRCI_COREPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_COREPLLCFG0_FSE_SHIFT            25
> +# define PRCI_COREPLLCFG0_FSE_MASK             (0x1 << PRCI_COREPLLCFG0_FSE_SHIFT)
> +# define PRCI_COREPLLCFG0_LOCK_SHIFT           31
> +# define PRCI_COREPLLCFG0_LOCK_MASK            (0x1 << PRCI_COREPLLCFG0_LOCK_SHIFT)
> +
> +/* DDRPLLCFG0 */
> +#define PRCI_DDRPLLCFG0_OFFSET                 0xc
> +# define PRCI_DDRPLLCFG0_DIVR_SHIFT            0
> +# define PRCI_DDRPLLCFG0_DIVR_MASK             (0x3f << PRCI_DDRPLLCFG0_DIVR_SHIFT)
> +# define PRCI_DDRPLLCFG0_DIVF_SHIFT            6
> +# define PRCI_DDRPLLCFG0_DIVF_MASK             (0x1ff << PRCI_DDRPLLCFG0_DIVF_SHIFT)
> +# define PRCI_DDRPLLCFG0_DIVQ_SHIFT            15
> +# define PRCI_DDRPLLCFG0_DIVQ_MASK             (0x7 << PRCI_DDRPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_DDRPLLCFG0_RANGE_SHIFT           18
> +# define PRCI_DDRPLLCFG0_RANGE_MASK            (0x7 << PRCI_DDRPLLCFG0_RANGE_SHIFT)
> +# define PRCI_DDRPLLCFG0_BYPASS_SHIFT          24
> +# define PRCI_DDRPLLCFG0_BYPASS_MASK           (0x1 << PRCI_DDRPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_DDRPLLCFG0_FSE_SHIFT             25
> +# define PRCI_DDRPLLCFG0_FSE_MASK              (0x1 << PRCI_DDRPLLCFG0_FSE_SHIFT)
> +# define PRCI_DDRPLLCFG0_LOCK_SHIFT            31
> +# define PRCI_DDRPLLCFG0_LOCK_MASK             (0x1 << PRCI_DDRPLLCFG0_LOCK_SHIFT)
> +
> +/* DDRPLLCFG1 */
> +#define PRCI_DDRPLLCFG1_OFFSET                 0x10
> +# define PRCI_DDRPLLCFG1_CKE_SHIFT             24
> +# define PRCI_DDRPLLCFG1_CKE_MASK              (0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
> +
> +/* GEMGXLPLLCFG0 */
> +#define PRCI_GEMGXLPLLCFG0_OFFSET              0x1c
> +# define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT         0
> +# define PRCI_GEMGXLPLLCFG0_DIVR_MASK          (0x3f << PRCI_GEMGXLPLLCFG0_DIVR_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_DIVF_SHIFT         6
> +# define PRCI_GEMGXLPLLCFG0_DIVF_MASK          (0x1ff << PRCI_GEMGXLPLLCFG0_DIVF_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT         15
> +# define PRCI_GEMGXLPLLCFG0_DIVQ_MASK          (0x7 << PRCI_GEMGXLPLLCFG0_DIVQ_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_RANGE_SHIFT                18
> +# define PRCI_GEMGXLPLLCFG0_RANGE_MASK         (0x7 << PRCI_GEMGXLPLLCFG0_RANGE_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT       24
> +# define PRCI_GEMGXLPLLCFG0_BYPASS_MASK                (0x1 << PRCI_GEMGXLPLLCFG0_BYPASS_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_FSE_SHIFT          25
> +# define PRCI_GEMGXLPLLCFG0_FSE_MASK           (0x1 << PRCI_GEMGXLPLLCFG0_FSE_SHIFT)
> +# define PRCI_GEMGXLPLLCFG0_LOCK_SHIFT         31
> +# define PRCI_GEMGXLPLLCFG0_LOCK_MASK          (0x1 << PRCI_GEMGXLPLLCFG0_LOCK_SHIFT)
> +
> +/* GEMGXLPLLCFG1 */
> +#define PRCI_GEMGXLPLLCFG1_OFFSET              0x20
> +# define PRCI_GEMGXLPLLCFG1_CKE_SHIFT          24
> +# define PRCI_GEMGXLPLLCFG1_CKE_MASK           (0x1 << PRCI_GEMGXLPLLCFG1_CKE_SHIFT)
> +
> +/* CORECLKSEL */
> +#define PRCI_CORECLKSEL_OFFSET                 0x24
> +# define PRCI_CORECLKSEL_CORECLKSEL_SHIFT      0
> +# define PRCI_CORECLKSEL_CORECLKSEL_MASK       (0x1 << PRCI_CORECLKSEL_CORECLKSEL_SHIFT)
> +
> +/* DEVICESRESETREG */
> +#define PRCI_DEVICESRESETREG_OFFSET                    0x28
> +# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT     0
> +# define PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_MASK      (0x1 << PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT      1
> +# define PRCI_DEVICESRESETREG_DDR_AXI_RST_N_MASK       (0x1 << PRCI_DEVICESRESETREG_DDR_AXI_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT      2
> +# define PRCI_DEVICESRESETREG_DDR_AHB_RST_N_MASK       (0x1 << PRCI_DEVICESRESETREG_DDR_AHB_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT      3
> +# define PRCI_DEVICESRESETREG_DDR_PHY_RST_N_MASK       (0x1 << PRCI_DEVICESRESETREG_DDR_PHY_RST_N_SHIFT)
> +# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT       5
> +# define PRCI_DEVICESRESETREG_GEMGXL_RST_N_MASK                (0x1 << PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT)
> +
> +/* CLKMUXSTATUSREG */
> +#define PRCI_CLKMUXSTATUSREG_OFFSET                    0x2c
> +# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT    1
> +# define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK     (0x1 << PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT)

I suspect a bunch of these macros could be simplified with FIELD_GET()
and FIELD_SET()?

> +
> +/*
> + * Private structures
> + */
> +
> +/**
> + * struct __prci_data - per-device-instance data
> + * @va: base virtual address of the PRCI IP block
> + * @hw_clks: encapsulates struct clk_hw records
> + *
> + * PRCI per-device instance data

What does PRCI stand for? The acronym is spelled out in Kconfig but not
this file.

> + */
> +struct __prci_data {
> +       void __iomem *va;
> +       struct clk_hw_onecell_data hw_clks;
> +};
> +
> +/**
> + * struct __prci_wrpll_data - WRPLL configuration and integration data
> + * @c: WRPLL current configuration record
> + * @enable_bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
> + * @disable_bypass: fn ptr to code to not bypass the WRPLL (or NULL)

We know it's a function pointer, perhaps just "bypass the WRPLL (if
applicable)" and "unbypass the WRLPLL (if applicable)".

> + * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
> + *
> + * @enable_bypass and @disable_bypass are used for WRPLL instances
> + * that contain a separate external glitchless clock mux downstream
> + * from the PLL.  The WRPLL internal bypass mux is not glitchless.
> + */
> +struct __prci_wrpll_data {
> +       struct analogbits_wrpll_cfg c;
> +       void (*enable_bypass)(struct __prci_data *pd);
> +       void (*disable_bypass)(struct __prci_data *pd);
> +       u8 cfg0_offs;
> +};
> +
> +/**
> + * struct __prci_clock - describes a clock device managed by PRCI
> + * @name: user-readable clock name string - should match the manual
> + * @parent_name: parent name for this clock
> + * @ops: struct clk_ops for the Linux clock framework to use for control
> + * @hw: Linux-private clock data
> + * @pwd: WRPLL-specific data, associated with this clock (if not NULL)
> + * @pd: PRCI-specific data associated with this clock (if not NULL)
> + *
> + * PRCI clock data.  Used by the PRCI driver to register PRCI-provided
> + * clocks to the Linux clock infrastructure.
> + */
> +struct __prci_clock {
> +       const char *name;
> +       const char *parent_name;
> +       const struct clk_ops *ops;
> +       struct clk_hw hw;
> +       struct __prci_wrpll_data *pwd;
> +       struct __prci_data *pd;
> +};
> +
> +#define clk_hw_to_prci_clock(pwd) container_of(pwd, struct __prci_clock, hw)
> +
> +/*
> + * Private functions
> + */
> +
> +/**
> + * __prci_readl() - read from a PRCI register
> + * @pd: PRCI context
> + * @offs: register offset to read from (in bytes, from PRCI base address)
> + *
> + * Read the register located at offset @offs from the base virtual
> + * address of the PRCI register target described by @pd, and return
> + * the value to the caller.
> + *
> + * Context: Any context.
> + *
> + * Return: the contents of the register described by @pd and @offs.
> + */
> +static u32 __prci_readl(struct __prci_data *pd, u32 offs)
> +{
> +       return readl_relaxed(pd->va + offs);
> +}
> +
> +static void __prci_writel(u32 v, u32 offs, struct __prci_data *pd)

No kernel-doc for this wrapper? Do we need kernel-doc for the read
wrapper?

> +{
> +       return writel_relaxed(v, pd->va + offs);
> +}
> +
> +/* WRPLL-related private functions */
> +
> +/**
> + * __prci_wrpll_unpack() - unpack WRPLL configuration registers into parameters
> + * @c: ptr to a struct analogbits_wrpll_cfg record to write config into
> + * @r: value read from the PRCI PLL configuration register
> + *
> + * Given a value @r read from an FU540 PRCI PLL configuration register,
> + * split it into fields and populate it into the WRPLL configuration record
> + * pointed to by @c.
> + *
> + * The COREPLLCFG0 macros are used below, but the other *PLLCFG0 macros
> + * have the same register layout.
> + *
> + * Context: Any context.
> + */
> +static void __prci_wrpll_unpack(struct analogbits_wrpll_cfg *c, u32 r)
> +{
> +       u32 v;
> +
> +       v = r & PRCI_COREPLLCFG0_DIVR_MASK;
> +       v >>= PRCI_COREPLLCFG0_DIVR_SHIFT;
> +       c->divr = v;
> +
> +       v = r & PRCI_COREPLLCFG0_DIVF_MASK;
> +       v >>= PRCI_COREPLLCFG0_DIVF_SHIFT;
> +       c->divf = v;
> +
> +       v = r & PRCI_COREPLLCFG0_DIVQ_MASK;
> +       v >>= PRCI_COREPLLCFG0_DIVQ_SHIFT;
> +       c->divq = v;
> +
> +       v = r & PRCI_COREPLLCFG0_RANGE_MASK;
> +       v >>= PRCI_COREPLLCFG0_RANGE_SHIFT;
> +       c->range = v;
> +
> +       c->flags &= (WRPLL_FLAGS_INT_FEEDBACK_MASK |
> +                    WRPLL_FLAGS_EXT_FEEDBACK_MASK);
> +
> +       /* external feedback mode not supported */
> +       c->flags |= WRPLL_FLAGS_INT_FEEDBACK_MASK;
> +}
> +
> +/**
> + * __prci_wrpll_pack() - pack PLL configuration parameters into a register value
> + * @c: pointer to a struct analogbits_wrpll_cfg record containing the PLL's cfg
> + *
> + * Using a set of WRPLL configuration values pointed to by @c,
> + * assemble a PRCI PLL configuration register value, and return it to
> + * the caller.
> + *
> + * Context: Any context.  Caller must ensure that the contents of the
> + *          record pointed to by @c do not change during the execution
> + *          of this function.
> + *
> + * Returns: a value suitable for writing into a PRCI PLL configuration
> + *          register
> + */
> +static u32 __prci_wrpll_pack(struct analogbits_wrpll_cfg * const c)

Shouldn't that be const struct analogbits_wrpll_cfg *c?

> +{
> +       u32 r = 0;
> +
> +       r |= c->divr << PRCI_COREPLLCFG0_DIVR_SHIFT;
> +       r |= c->divf << PRCI_COREPLLCFG0_DIVF_SHIFT;
> +       r |= c->divq << PRCI_COREPLLCFG0_DIVQ_SHIFT;
> +       r |= c->range << PRCI_COREPLLCFG0_RANGE_SHIFT;
> +
> +       /* external feedback mode not supported */
> +       r |= PRCI_COREPLLCFG0_FSE_MASK;
> +
> +       return r;
> +}
> +
> +/**
> + * __prci_wrpll_read_cfg() - read the WRPLL configuration from the PRCI
> + * @pd: PRCI context
> + * @pwd: PRCI WRPLL metadata
> + *
> + * Read the current configuration of the PLL identified by @pwd from
> + * the PRCI identified by @pd, and store it into the local configuration
> + * cache in @pwd.
> + *
> + * Context: Any context.  Caller must prevent the records pointed to by
> + *          @pd and @pwd from changing during execution.
> + */
> +static void __prci_wrpll_read_cfg(struct __prci_data *pd,

Maybe call this __prci_wrpll_cache_cfg?

> +                                 struct __prci_wrpll_data *pwd)
> +{
> +       __prci_wrpll_unpack(&pwd->c, __prci_readl(pd, pwd->cfg0_offs));
> +}
> +
> +/**
> + * __prci_wrpll_write_cfg() - write WRPLL configuration into the PRCI
> + * @pd: PRCI context
> + * @pwd: PRCI WRPLL metadata
> + * @c: WRPLL configuration record to write
> + *
> + * Write the WRPLL configuration described by @c into the WRPLL
> + * configuration register identified by @pwd in the PRCI instance
> + * described by @c.  Make a cached copy of the WRPLL's current
> + * configuration so it can be used by other code.
> + *
> + * Context: Any context.  Caller must prevent the records pointed to by
> + *          @pd and @pwd from changing during execution.
> + */
> +static void __prci_wrpll_write_cfg(struct __prci_data *pd,

const pd?

> +                                  struct __prci_wrpll_data *pwd,
> +                                  struct analogbits_wrpll_cfg *c)
> +{
> +       __prci_writel(__prci_wrpll_pack(c), pwd->cfg0_offs, pd);
> +
> +       memcpy(&pwd->c, c, sizeof(struct analogbits_wrpll_cfg));
> +}
> +
> +/* Core clock mux control */
> +
> +/**
> + * __prci_coreclksel_use_hfclk() - switch the CORECLK mux to output HFCLK
> + * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
> + *
> + * Switch the CORECLK mux to the HFCLK input source; return once complete.
> + *
> + * Context: Any context.  Caller must prevent concurrent changes to the
> + *          PRCI_CORECLKSEL_OFFSET register.
> + */
> +static void __prci_coreclksel_use_hfclk(struct __prci_data *pd)
> +{
> +       u32 r;
> +
> +       r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
> +       r |= PRCI_CORECLKSEL_CORECLKSEL_MASK;
> +       __prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
> +
> +       r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
> +}
> +
> +/**
> + * __prci_coreclksel_use_corepll() - switch the CORECLK mux to output COREPLL
> + * @pd: struct __prci_data * for the PRCI containing the CORECLK mux reg
> + *
> + * Switch the CORECLK mux to the PLL output clock; return once complete.
> + *
> + * Context: Any context.  Caller must prevent concurrent changes to the
> + *          PRCI_CORECLKSEL_OFFSET register.
> + */
> +static void __prci_coreclksel_use_corepll(struct __prci_data *pd)
> +{
> +       u32 r;
> +
> +       r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET);
> +       r &= ~PRCI_CORECLKSEL_CORECLKSEL_MASK;
> +       __prci_writel(r, PRCI_CORECLKSEL_OFFSET, pd);
> +
> +       r = __prci_readl(pd, PRCI_CORECLKSEL_OFFSET); /* barrier */
> +}
> +
> +/*
> + * Linux clock framework integration
> + *
> + * See the Linux clock framework documentation for more information on
> + * these functions.
> + */

Seems like a useless comment.

> +
> +static unsigned long sifive_fu540_prci_wrpll_recalc_rate(struct clk_hw *hw,
> +                                                        unsigned long parent_rate)
> +{
> +       struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +       struct __prci_wrpll_data *pwd = pc->pwd;
> +
> +       return analogbits_wrpll_calc_output_rate(&pwd->c, parent_rate);
> +}
> +
> +static long sifive_fu540_prci_wrpll_round_rate(struct clk_hw *hw,
> +                                              unsigned long rate,
> +                                              unsigned long *parent_rate)
> +{
> +       struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +       struct __prci_wrpll_data *pwd = pc->pwd;
> +       struct analogbits_wrpll_cfg c;
> +
> +       memcpy(&c, &pwd->c, sizeof(c));
> +
> +       analogbits_wrpll_configure_for_rate(&c, rate, *parent_rate);
> +
> +       return analogbits_wrpll_calc_output_rate(&c, *parent_rate);
> +}
> +
> +static int sifive_fu540_prci_wrpll_set_rate(struct clk_hw *hw,
> +                                           unsigned long rate,
> +                                           unsigned long parent_rate)
> +{
> +       struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +       struct __prci_wrpll_data *pwd = pc->pwd;
> +       struct __prci_data *pd = pc->pd;
> +       int r;
> +
> +       r = analogbits_wrpll_configure_for_rate(&pwd->c, rate, parent_rate);
> +       if (r)
> +               return r;
> +
> +       if (pwd->enable_bypass)
> +               pwd->enable_bypass(pd);
> +
> +       __prci_wrpll_write_cfg(pd, pwd, &pwd->c);
> +
> +       udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));

Can it be usleep_range()? This is a set_rate path after all.

> +
> +       if (pwd->disable_bypass)
> +               pwd->disable_bypass(pd);
> +
> +       return 0;
> +}
[...]
> +/* TLCLKSEL clock integration */
> +
> +static unsigned long sifive_fu540_prci_tlclksel_recalc_rate(struct clk_hw *hw,
> +                                                           unsigned long parent_rate)
> +{
> +       struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +       struct __prci_data *pd = pc->pd;
> +       u32 v;
> +       u8 div;
> +
> +       v = __prci_readl(pd, PRCI_CLKMUXSTATUSREG_OFFSET);
> +       v &= PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK;
> +       div = v ? 1 : 2;

I thought there was a helper for this 1 vs. 2 descision?

> +
> +       return div_u64(parent_rate, div);

Why use div_u64? parent_rate is unsigned long.

> +}
> +
> +static const struct clk_ops sifive_fu540_prci_tlclksel_clk_ops = {
> +       .recalc_rate = sifive_fu540_prci_tlclksel_recalc_rate,
> +};
> +
> +/*
> + * PRCI integration data for each WRPLL instance
> + */
> +
> +static struct __prci_wrpll_data __prci_corepll_data = {
> +       .cfg0_offs = PRCI_COREPLLCFG0_OFFSET,
> +       .enable_bypass = __prci_coreclksel_use_hfclk,
> +       .disable_bypass = __prci_coreclksel_use_corepll,
> +};
> +
> +static struct __prci_wrpll_data __prci_ddrpll_data = {
> +       .cfg0_offs = PRCI_DDRPLLCFG0_OFFSET,
> +};
> +
> +static struct __prci_wrpll_data __prci_gemgxlpll_data = {
> +       .cfg0_offs = PRCI_GEMGXLPLLCFG0_OFFSET,
> +};
> +
> +/*
> + * List of clock controls provided by the PRCI
> + */
> +
> +static struct __prci_clock __prci_init_clocks[] = {
> +       [PRCI_CLK_COREPLL] = {
> +               .name = "corepll",
> +               .parent_name = "hfclk",
> +               .ops = &sifive_fu540_prci_wrpll_clk_ops,
> +               .pwd = &__prci_corepll_data,

Why is this a pointer stored in here? Put another way, why can't we
store the whole structure for the prci_wrpll_data directly inside the
prci_clock structure?

> +       },
> +       [PRCI_CLK_DDRPLL] = {
> +               .name = "ddrpll",
> +               .parent_name = "hfclk",
> +               .ops = &sifive_fu540_prci_wrpll_ro_clk_ops,
> +               .pwd = &__prci_ddrpll_data,
> +       },
> +       [PRCI_CLK_GEMGXLPLL] = {
> +               .name = "gemgxlpll",
> +               .parent_name = "hfclk",
> +               .ops = &sifive_fu540_prci_wrpll_clk_ops,
> +               .pwd = &__prci_gemgxlpll_data,
> +       },
> +       [PRCI_CLK_TLCLK] = {
> +               .name = "tlclk",
> +               .parent_name = "corepll",
> +               .ops = &sifive_fu540_prci_tlclksel_clk_ops,
> +       },
> +};
> +
> +/**
> + * __prci_register_clocks() - register clock controls in the PRCI with Linux
> + * @dev: Linux struct device *

Maybe 'device registering clks'. Or just collapse this into 'probe'
below.

> + *
> + * Register the list of clock controls described in __prci_init_plls[] with
> + * the Linux clock framework.
> + *
> + * Return: 0 upon success or a negative error code upon failure.
> + */
> +static int __prci_register_clocks(struct device *dev, struct __prci_data *pd)
> +{
> +       struct clk_init_data init;
> +       struct __prci_clock *pic;
> +       int parent_count, i, clk_hw_count, r;
> +
> +       parent_count = of_clk_get_parent_count(dev->of_node);
> +       if (parent_count != EXPECTED_CLK_PARENT_COUNT) {

We don't need to validate DT. That's what DT schemas are for.

> +               dev_err(dev, "expected only two parent clocks, found %d\n",
> +                       parent_count);
> +               return -EINVAL;
> +       }
> +
> +       memset(&init, 0, sizeof(struct clk_init_data));

sizeof(init)? Or just struct clk_init_data init = { };

> +
> +       /* Register PLLs */
> +       clk_hw_count = sizeof(__prci_init_clocks) / sizeof(struct __prci_clock);

Is this ARRAY_SIZE()?

> +
> +       for (i = 0; i < clk_hw_count; ++i) {
> +               pic = &__prci_init_clocks[i];
> +
> +               init.name = pic->name;
> +               init.parent_names = &pic->parent_name;
> +               init.num_parents = 1;
> +               init.ops = pic->ops;
> +               pic->hw.init = &init;
> +
> +               pic->pd = pd;
> +
> +               if (pic->pwd)
> +                       __prci_wrpll_read_cfg(pd, pic->pwd);
> +
> +               r = devm_clk_hw_register(dev, &pic->hw);
> +               if (r) {
> +                       dev_warn(dev, "Failed to register clock %s: %d\n",
> +                                init.name, r);
> +                       return r;
> +               }
> +
> +               r = clk_hw_register_clkdev(&pic->hw, pic->name, dev_name(dev));
> +               if (r) {
> +                       dev_warn(dev, "Failed to register clkdev for %s: %d\n",

But we return right after, and don't use devm_() to register other
clkdevs so we leak some here? I hope clkdev can be removed from this
driver.

> +                                init.name, r);
> +                       return r;
> +               }
> +
> +               pd->hw_clks.hws[i] = &pic->hw;
> +       }
> +
> +       pd->hw_clks.num = i;
> +
> +       r = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                       &pd->hw_clks);
> +       if (r) {
> +               dev_err(dev, "could not add hw_provider: %d\n", r);
> +               return r;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * Linux device model integration
> + *
> + * See the Linux device model documentation for more information about
> + * these functions.
> + */

Can you remove this comment? It doesn't seem worthwhile.

Re: [PATCH v3 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver

[Stephen Boyd]

Quoting Paul Walmsley (2019-04-11 01:27:34)
> Add DT binding documentation for the Linux driver for the SiFive
> PRCI clock & reset control IP block, as found on the SiFive
> FU540 chip.
> 
> This version includes changes requested by Stephen Boyd
> <sboyd@kernel.org> and Rob Herring <robh@kernel.org>, and
> fixes some errors in the initial version.
> 
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Megan Wachs <megan@sifive.com>
> Cc: linux-clk@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---

Applied to clk-next

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Stephen Boyd]

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

[Paul Walmsley]

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 000000000000..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 000000000000..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 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;

To be equivalent to the original code, we'd need to write:

       switch (post_divr_freq) {
       case 0 ... 10999999:
               return 1;
       case 11000000 ... 17999999:
               return 2;
(etc.)

In any case, it's been changed to use the gcc case range operator.

> > +
> > +       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?

Done.

> > +{
> > +       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.
  
> > + */
> > +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.

> 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.

> > +{
> > +       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?

It's just the application of that first principle mentioned above.

> > +
> > +       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?

> > +
> > +       /* 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?

Thanks, changed.

> > +
> > +       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 ..."

As you wish.

> > + * 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?

Same rationale and commentary as above.

> > +                                       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?

Done.

> > +       }
> > +
> > +       /* 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.

Done.

> > +
> > +       /* 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)

As you wish.

> > +               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?

No.  I've removed the distraction.

> > +               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?

Done.

> > +
> > +       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?

Changed.

> > +                                               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?

Changed.

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

Done.

> > +       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?

Changed.

> > +{
> > +       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.

Done.

> > + */
> > +
> > +#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?

Thanks, fixed.

> > + * 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?

To me BIT() is clearer and more concise.  However, please let me know if 
the use of the FIELD_*() macros is important to you, and I will change it.

> > +
> > +/**
> > + * 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.

Thanks, done.

> > + */
> > +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.

I don't have a strong preference either way.  I've changed it to remove 
the analogbits prefix as you request.


- Paul

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Paul Walmsley]

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 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Paul Walmsley]

Hi Atish,

On Sat, 27 Apr 2019, Atish Patra wrote:

> On 4/11/19 1:28 AM, Paul Walmsley wrote:
> > Add driver code for the SiFive FU540 PRCI IP block.  This IP block
> > handles reset and clock control for the SiFive FU540 device and
> > implements SoC-level clock tree controls and dividers.

[...]

> > +static const struct of_device_id sifive_fu540_prci_of_match[] = {
> > +	{ .compatible = "sifive,fu540-c000-prci", },
> 
> All the existing unleashed devices have prci clock compatible string as
> "sifive,aloeprci0" or "sifive,ux00prci0". Should it be added to maintain
> backward compatibility?

As you note, just adding the old (unreviewed) compatible string isn't 
enough.

> Even after adding the compatible string (just for my testing purpose), I get
> this while booting.
> 
> [    0.104571] sifive-fu540-prci 10000000.prci: expected only two parent
> clocks, found 1
> [    0.112460] sifive-fu540-prci 10000000.prci: could not register clocks: -22
> [    0.119499] sifive-fu540-prci: probe of 10000000.prci failed with error -22
> 
> Looking at the DT entries, your DT patch has
> 
> +		prci: clock-controller@10000000 {
> +			compatible = "sifive,fu540-c000-prci";
> +			reg = <0x0 0x10000000 0x0 0x1000>;
> +			clocks = <&hfclk>, <&rtcclk>;
> +			#clock-cells = <1>;
> +		};
> 
> 
> while current DT from FSBL
> (https://github.com/sifive/freedom-u540-c000-bootloader/blob/master/fsbl/ux00_fsbl.dts)
> 
> prci: prci@10000000 {
> 			compatible = "sifive,aloeprci0", "sifive,ux00prci0";
> 			reg = <0x0 0x10000000 0x0 0x1000>;
> 			reg-names = "control";
> 			clocks = <&refclk>;
> 			#clock-cells = <1>;
> 		};
> 
> This seems to be the cause of error. It looks like this patch needs a complete
> different DT (your DT patch) than FSBL provides.

That's right.  That old data was completely out of tree and unreviewed.  
It's part of the reason why we're going through the process of posting DT 
data to the kernel and devicetree lists and getting that data reviewed:

https://lore.kernel.org/linux-riscv/20190411084242.4999-1-paul.walmsley@sifive.com/

> This means everybody must upgrade the FSBL to use your DT patch in their
> boards once this driver is merged. Is this okay?

People can continue to use the out-of-tree DT data if they want.  They'll 
just have to continue to patch their kernels to add out-of-tree drivers, 
as they do now.

Otherwise, if people want to use the upstream PRCI driver in the upstream 
kernel, then it's necessary to use DT data that aligns with what's in the 
upstream binding documentation.


- Paul

Re: [PATCH v3 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Atish Patra]

On 4/29/19 11:20 PM, Paul Walmsley wrote:
> Hi Atish,
> 
> On Sat, 27 Apr 2019, Atish Patra wrote:
> 
>> On 4/11/19 1:28 AM, Paul Walmsley wrote:
>>> Add driver code for the SiFive FU540 PRCI IP block.  This IP block
>>> handles reset and clock control for the SiFive FU540 device and
>>> implements SoC-level clock tree controls and dividers.
> 
> [...]
> 
>>> +static const struct of_device_id sifive_fu540_prci_of_match[] = {
>>> +	{ .compatible = "sifive,fu540-c000-prci", },
>>
>> All the existing unleashed devices have prci clock compatible string as
>> "sifive,aloeprci0" or "sifive,ux00prci0". Should it be added to maintain
>> backward compatibility?
> 
> As you note, just adding the old (unreviewed) compatible string isn't
> enough.
> 
>> Even after adding the compatible string (just for my testing purpose), I get
>> this while booting.
>>
>> [    0.104571] sifive-fu540-prci 10000000.prci: expected only two parent
>> clocks, found 1
>> [    0.112460] sifive-fu540-prci 10000000.prci: could not register clocks: -22
>> [    0.119499] sifive-fu540-prci: probe of 10000000.prci failed with error -22
>>
>> Looking at the DT entries, your DT patch has
>>
>> +		prci: clock-controller@10000000 {
>> +			compatible = "sifive,fu540-c000-prci";
>> +			reg = <0x0 0x10000000 0x0 0x1000>;
>> +			clocks = <&hfclk>, <&rtcclk>;
>> +			#clock-cells = <1>;
>> +		};
>>
>>
>> while current DT from FSBL
>> (https://github.com/sifive/freedom-u540-c000-bootloader/blob/master/fsbl/ux00_fsbl.dts)
>>
>> prci: prci@10000000 {
>> 			compatible = "sifive,aloeprci0", "sifive,ux00prci0";
>> 			reg = <0x0 0x10000000 0x0 0x1000>;
>> 			reg-names = "control";
>> 			clocks = <&refclk>;
>> 			#clock-cells = <1>;
>> 		};
>>
>> This seems to be the cause of error. It looks like this patch needs a complete
>> different DT (your DT patch) than FSBL provides.
> 
> That's right.  That old data was completely out of tree and unreviewed.
> It's part of the reason why we're going through the process of posting DT
> data to the kernel and devicetree lists and getting that data reviewed:
> 
> https://lore.kernel.org/linux-riscv/20190411084242.4999-1-paul.walmsley@sifive.com/
> 
>> This means everybody must upgrade the FSBL to use your DT patch in their
>> boards once this driver is merged. Is this okay?
> 
> People can continue to use the out-of-tree DT data if they want.  They'll
> just have to continue to patch their kernels to add out-of-tree drivers,
> as they do now.
> 

There were some concerns about the breaking the existing setup in the past.

> Otherwise, if people want to use the upstream PRCI driver in the upstream
> kernel, then it's necessary to use DT data that aligns with what's in the
> upstream binding documentation.
> 

Personally, it makes sense to me. I am okay with upgrading FSBL to 
update the DT once the patches are in mainline. In fact, I used to do 
that for topology patch series. This will help to add any new DT entry 
in future as well.

However, if SiFive can share a prebuilt FSBL image for everybody to 
upgrade, that would be very helpful.

Regards,
Atish
> 
> - Paul
> 

Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

[Stephen Boyd]

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

[Paul Walmsley]

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

[Stephen Boyd]

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 ... 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;
> 
> To be equivalent to the original code, we'd need to write:
> 
>        switch (post_divr_freq) {
>        case 0 ... 10999999:
>                return 1;
>        case 11000000 ... 17999999:
>                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 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?
> 
> To me BIT() is clearer and more concise.  However, please let me know if 
> the use of the FIELD_*() macros is important to you, and I will change it.

I'm not strong on it so up to you.

> > > + */
> > > +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.
> 
> I don't have a strong preference either way.  I've changed it to remove 
> the analogbits prefix as you request.
> 

Alright sounds good!