* [PATCH v2 0/3] clk: add driver for the SiFive FU540 PRCI and PLLs it controls
@ 2018-10-20 13:50 Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library Paul Walmsley
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-10-20 13:50 UTC (permalink / raw)
To: linux-clk, devicetree
Cc: Paul Walmsley, Michael Turquette, Stephen Boyd, Albert Ou,
Wesley W . Terpstra, Palmer Dabbelt, Megan Wachs, Rob Herring,
Mark Rutland, linux-riscv, linux-kernel
Add a driver for the SiFive FU540 PRCI IP block, which handles clock and
some device reset control for the SiFive FU540 chip. Also add a driver-
independent library for the Analog Bits Wide-Range PLL (WRPLL), used by
the PRCI driver to monitor and control the WRPLL instances on the FU540
chip.
It's been a long time since I've worked on Linux clock code, so Mike &
Stephen, I'd appreciate a close look at the PRCI driver to make sure
what it's doing makes sense.
Boot-tested on a SiFive HiFive Unleashed board. COREPLL rate changes
and notification were also tested on the same board.
This second version corrects the DT binding documentation patch; an
out-of-date version of that patch was sent with the first version.
This patch series is also available at:
https://github.com/sifive/riscv-linux/tree/dev/paulw/prci-v4.19-rc7
Paul Walmsley (3):
clk: analogbits: add Wide-Range PLL library
dt-bindings: clk: add documentation for the SiFive PRCI driver
clk: sifive: add a driver for the SiFive FU540 PRCI IP block
.../bindings/clock/sifive/fu540-prci.txt | 43 ++
MAINTAINERS | 6 +
drivers/clk/Kconfig | 2 +
drivers/clk/Makefile | 2 +
drivers/clk/analogbits/Kconfig | 3 +
drivers/clk/analogbits/Makefile | 2 +
drivers/clk/analogbits/wrpll-cln28hpc.c | 387 +++++++++++
drivers/clk/sifive/Kconfig | 18 +
drivers/clk/sifive/Makefile | 1 +
drivers/clk/sifive/fu540-prci.c | 634 ++++++++++++++++++
include/linux/clk/analogbits-wrpll-cln28hpc.h | 99 +++
include/linux/clk/sifive-fu540-prci.h | 27 +
12 files changed, 1224 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
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 drivers/clk/sifive/Kconfig
create mode 100644 drivers/clk/sifive/Makefile
create mode 100644 drivers/clk/sifive/fu540-prci.c
create mode 100644 include/linux/clk/analogbits-wrpll-cln28hpc.h
create mode 100644 include/linux/clk/sifive-fu540-prci.h
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: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-clk@vger.kernel.org
--
2.19.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library
2018-10-20 13:50 [PATCH v2 0/3] clk: add driver for the SiFive FU540 PRCI and PLLs it controls Paul Walmsley
@ 2018-10-20 13:50 ` Paul Walmsley
2018-10-25 7:47 ` Stephen Boyd
2018-10-20 13:50 ` [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block Paul Walmsley
2 siblings, 1 reply; 14+ messages in thread
From: Paul Walmsley @ 2018-10-20 13:50 UTC (permalink / raw)
To: linux-clk, devicetree
Cc: Paul Walmsley, Wesley Terpstra, Palmer Dabbelt,
Michael Turquette, Stephen Boyd, Megan Wachs, linux-kernel,
Paul Walmsley
Add common library code for the Analog Bits Wide-Range PLL (WRPLL) 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
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
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
MAINTAINERS | 6 +
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 1 +
drivers/clk/analogbits/Kconfig | 3 +
drivers/clk/analogbits/Makefile | 2 +
drivers/clk/analogbits/wrpll-cln28hpc.c | 387 ++++++++++++++++++
include/linux/clk/analogbits-wrpll-cln28hpc.h | 99 +++++
7 files changed, 499 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 48a65c3a4189..02c7b2b72a6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -911,6 +911,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 292056bbb30e..a34ccfcb54ac 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -284,6 +284,7 @@ config COMMON_CLK_STM32H7
Support for stm32h7 SoC family 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 a84c5573cabe..d59c11ddfc5e 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -62,6 +62,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..5ccf7902f6e2
--- /dev/null
+++ b/drivers/clk/analogbits/Kconfig
@@ -0,0 +1,3 @@
+config CLK_ANALOGBITS_WRPLL_CLN28HPC
+ bool
+
diff --git a/drivers/clk/analogbits/Makefile b/drivers/clk/analogbits/Makefile
new file mode 100644
index 000000000000..d3c102c8b4e0
--- /dev/null
+++ b/drivers/clk/analogbits/Makefile
@@ -0,0 +1,2 @@
+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..ebdef859cbf5
--- /dev/null
+++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
@@ -0,0 +1,387 @@
+// 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.
+ *
+ * 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"
+ */
+
+#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;
+}
+
+/*
+ * Public functions
+ */
+
+/**
+ * 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, fbcfg;
+ u8 fbdiv, divq, best_r, r;
+
+ if (!c)
+ return -1;
+
+ if (c->flags == 0) {
+ WARN(1, "%s called with uninitialized PLL config", __func__);
+ return -1;
+ }
+
+ fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
+ if ((c->flags & fbcfg) == fbcfg) {
+ WARN(1, "%s called with invalid PLL config", __func__);
+ return -1;
+ }
+
+ if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
+ WARN(1, "%s: external feedback mode not currently supported",
+ __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..cc4268f16067
--- /dev/null
+++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
@@ -0,0 +1,99 @@
+/* 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;
+ u32 _output_rate_cache[DIVQ_VALUES];
+ unsigned long _parent_rate;
+ u8 _max_r;
+ u8 _init_r;
+};
+
+/*
+ * Function prototypes
+ */
+
+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.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver
2018-10-20 13:50 [PATCH v2 0/3] clk: add driver for the SiFive FU540 PRCI and PLLs it controls Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library Paul Walmsley
@ 2018-10-20 13:50 ` Paul Walmsley
2018-10-24 18:47 ` Rob Herring
2018-11-06 17:33 ` Stephen Boyd
2018-10-20 13:50 ` [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block Paul Walmsley
2 siblings, 2 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-10-20 13:50 UTC (permalink / raw)
To: linux-clk, devicetree
Cc: Paul Walmsley, Michael Turquette, Stephen Boyd, Rob Herring,
Mark Rutland, Palmer Dabbelt, Megan Wachs, linux-riscv,
linux-kernel, 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.
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
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
v2: remove out-of-date example, add documentation for the compatible
string and for the required PCB clock nodes
.../bindings/clock/sifive/fu540-prci.txt | 43 +++++++++++++++++++
1 file changed, 43 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..d7c1e83fa5ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
@@ -0,0 +1,43 @@
+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<version>". As of the time this
+ file was written, only one value is supported:
+ "sifive,fu540-c000-prci0"
+- 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/linux/clk/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.
+
+Examples:
+
+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";
+};
+prci0: prci@10000000 {
+ compatible = "sifive,fu540-c000-prci0";
+ reg = <0x0 0x10000000 0x0 0x1000>;
+ clocks = <&hfclk>, <&rtcclk>;
+ #clock-cells = <1>;
+};
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block
2018-10-20 13:50 [PATCH v2 0/3] clk: add driver for the SiFive FU540 PRCI and PLLs it controls Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver Paul Walmsley
@ 2018-10-20 13:50 ` Paul Walmsley
2018-11-21 16:35 ` Stephen Boyd
2 siblings, 1 reply; 14+ messages in thread
From: Paul Walmsley @ 2018-10-20 13:50 UTC (permalink / raw)
To: linux-clk, devicetree
Cc: Paul Walmsley, Michael Turquette, Stephen Boyd, Albert Ou,
Wesley W . Terpstra, Palmer Dabbelt, Megan Wachs, linux-riscv,
linux-kernel, 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.
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
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 1 +
drivers/clk/sifive/Kconfig | 18 +
drivers/clk/sifive/Makefile | 1 +
drivers/clk/sifive/fu540-prci.c | 634 ++++++++++++++++++++++++++
include/linux/clk/sifive-fu540-prci.h | 27 ++
6 files changed, 682 insertions(+)
create mode 100644 drivers/clk/sifive/Kconfig
create mode 100644 drivers/clk/sifive/Makefile
create mode 100644 drivers/clk/sifive/fu540-prci.c
create mode 100644 include/linux/clk/sifive-fu540-prci.h
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index a34ccfcb54ac..0af5f6843a2f 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -295,6 +295,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 d59c11ddfc5e..8b07ebab37f8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -91,6 +91,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..870cb8333648
--- /dev/null
+++ b/drivers/clk/sifive/fu540-prci.c
@@ -0,0 +1,634 @@
+// 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 <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/clk/analogbits-wrpll-cln28hpc.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/log2.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clk/sifive-fu540-prci.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
+ * @bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
+ * @no_bypass: fn ptr to code to not bypass the WRPLL (if applicable; else NULL)
+ * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
+ *
+ * @bypass and @no_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 (*bypass)(struct __prci_data *pd);
+ void (*no_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);
+
+ if (r & PRCI_COREPLLCFG0_FSE_MASK)
+ c->flags |= WRPLL_FLAGS_INT_FEEDBACK_MASK;
+ else
+ c->flags |= WRPLL_FLAGS_EXT_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 *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;
+ if (c->flags & WRPLL_FLAGS_INT_FEEDBACK_MASK)
+ 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 -ERANGE;
+
+ if (pwd->bypass)
+ pwd->bypass(pd);
+
+ __prci_wrpll_write_cfg(pd, pwd, &pwd->c);
+
+ udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));
+
+ if (pwd->no_bypass)
+ pwd->no_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,
+ .bypass = __prci_coreclksel_use_hfclk,
+ .no_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 Linux error code upon failure.
+ */
+static int __prci_register_clocks(struct device *dev)
+{
+ struct __prci_data *pd = dev_get_drvdata(dev);
+ 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 two parent clocks, only 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 = (const char *[]) { pic->parent_name };
+ init.num_parents = 1;
+ init.ops = pic->ops;
+ init.flags = CLK_IGNORE_UNUSED;
+ 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 -EINVAL;
+ }
+
+ 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(struct __prci_data), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, pd);
+
+ 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);
+ if (r) {
+ dev_err(dev, "could not register clocks: %d\n", r);
+ return -EINVAL;
+ }
+
+ dev_info(dev, "SiFive FU540 PRCI probed\n");
+
+ return 0;
+}
+
+static const struct of_device_id sifive_fu540_prci_of_match[] = {
+ { .compatible = "sifive,fu540-c000-prci0", },
+ {}
+};
+
+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);
diff --git a/include/linux/clk/sifive-fu540-prci.h b/include/linux/clk/sifive-fu540-prci.h
new file mode 100644
index 000000000000..5d03eae7d4ef
--- /dev/null
+++ b/include/linux/clk/sifive-fu540-prci.h
@@ -0,0 +1,27 @@
+/* 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_SIFIVE_FU540_PRCI_H
+#define __LINUX_CLK_SIFIVE_FU540_PRCI_H
+
+/* Clock indexes for use by Device Tree data */
+
+#define PRCI_CLK_COREPLL 0
+#define PRCI_CLK_DDRPLL 1
+#define PRCI_CLK_GEMGXLPLL 2
+#define PRCI_CLK_TLCLK 3
+
+#endif
--
2.19.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver
2018-10-20 13:50 ` [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver Paul Walmsley
@ 2018-10-24 18:47 ` Rob Herring
2018-11-16 23:18 ` Paul Walmsley
2018-11-06 17:33 ` Stephen Boyd
1 sibling, 1 reply; 14+ messages in thread
From: Rob Herring @ 2018-10-24 18:47 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-clk, devicetree, Michael Turquette, Stephen Boyd,
Mark Rutland, Palmer Dabbelt, Megan Wachs, linux-riscv,
linux-kernel, Paul Walmsley
On Sat, Oct 20, 2018 at 06:50:23AM -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.
>
> 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
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
> v2: remove out-of-date example, add documentation for the compatible
> string and for the required PCB clock nodes
>
> .../bindings/clock/sifive/fu540-prci.txt | 43 +++++++++++++++++++
> 1 file changed, 43 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..d7c1e83fa5ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
> @@ -0,0 +1,43 @@
> +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<version>". As of the time this
> + file was written, only one value is supported:
> + "sifive,fu540-c000-prci0"
What happens with this depends on the discussion on the other bindings.
Though here you are inconsistent without a fallback. Of course, I've
never seen a clock controller be the same across SoCs.
> +- 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/linux/clk/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.
> +
> +Examples:
> +
> +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";
> +};
> +prci0: prci@10000000 {
clock-controller@...
> + compatible = "sifive,fu540-c000-prci0";
> + reg = <0x0 0x10000000 0x0 0x1000>;
> + clocks = <&hfclk>, <&rtcclk>;
> + #clock-cells = <1>;
> +};
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library
2018-10-20 13:50 ` [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library Paul Walmsley
@ 2018-10-25 7:47 ` Stephen Boyd
2018-11-09 1:01 ` Paul Walmsley
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-10-25 7:47 UTC (permalink / raw)
To: Paul Walmsley, devicetree, linux-clk
Cc: Paul Walmsley, Wesley Terpstra, Palmer Dabbelt,
Michael Turquette, Megan Wachs, linux-kernel, Paul Walmsley
Quoting Paul Walmsley (2018-10-20 06:50:22)
> Add common library code for the Analog Bits Wide-Range PLL (WRPLL) 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
I will take a deeper review next week.
>
> 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
Weird tab ^?
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
> new file mode 100644
> index 000000000000..ebdef859cbf5
> --- /dev/null
> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
> @@ -0,0 +1,387 @@
> +// 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.
> + *
Can you drop these two paragraphs? It's duplicated from the SPDX tag.
> + * 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"
Any html links?
> + */
> +
> +#include <linux/bug.h>
Is this used?
> +#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.
Would that ever be needed? Presumably the flag isn't changing.
> + *
> + * 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)
> + */
> +static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)
Nitpick: Why so much underscore in this code?
> +{
> + u64 s;
> + u8 divq = 0;
> +
> + if (!vco_rate) {
> + WARN_ON(1);
> + goto wcd_out;
Just return here, There aren't any locks so no need for goto.
Also, don't check anything? The caller is bad if this happens, and this
is a static function so it can pretty much be found by visual
inspection or during development?
> + }
> +
> + 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;
Does this need to be a u8? It makes the min_t below required instead of
using an unsigned int.
> +
> + if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
> + return -1;
Please pick some valid error code instead of -1 (I thought checkpatch
complained about this?). Is this case even possible? It would be nicer
if this function just returned void.
> +
> + 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;
> +}
> +
> +/*
> + * Public functions
> + */
> +
> +/**
> + * 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, fbcfg;
> + u8 fbdiv, divq, best_r, r;
> +
> + if (!c)
> + return -1;
-EINVAL? Of course, it's probably better to just not care and blow up if
the user of the API passes in NULL.
> +
> + if (c->flags == 0) {
> + WARN(1, "%s called with uninitialized PLL config", __func__);
Do we really need the stacktrace in this case? Or can this be downgraded
to a pr_warn() and return some error value that isn't -1?
> + return -1;
> + }
> +
> + fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
> + if ((c->flags & fbcfg) == fbcfg) {
> + WARN(1, "%s called with invalid PLL config", __func__);
> + return -1;
> + }
> +
> + if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
> + WARN(1, "%s: external feedback mode not currently supported",
> + __func__);
> + return -1;
> + }
Like a bunch of this stuff, why are we checking things that the caller
should know to do itself? I'd prefer we keep things simple and assume
the caller knows what they're doing.
> +
> + /* 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)
const c?
> +{
> + return MAX_LOCK_US;
And we're planning to change this in the future?
> +}
> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> new file mode 100644
> index 000000000000..cc4268f16067
> --- /dev/null
> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> @@ -0,0 +1,99 @@
> +/* 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.
Ah ok. Are the functions also done this way for a private/public API
split. We don't typically care about this in the kernel, just mark it
static if it's private and non-static if it's public for functions.
Underscores just make lines longer. And for struct members we have
kernel doc 'private' tag that can tell users to not do something. Or
worst case, an opaque pointer is stored and only defined in the C file.
> + */
> +struct analogbits_wrpll_cfg {
> + u8 divr;
> + u8 divq;
> + u8 range;
> + u8 flags;
> + u16 divf;
> + u32 _output_rate_cache[DIVQ_VALUES];
> + unsigned long _parent_rate;
> + u8 _max_r;
> + u8 _init_r;
> +};
> +
> +/*
> + * Function prototypes
> + */
Please don't have these types of comments. They don't help.
> +
> +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.19.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver
2018-10-20 13:50 ` [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver Paul Walmsley
2018-10-24 18:47 ` Rob Herring
@ 2018-11-06 17:33 ` Stephen Boyd
2018-11-07 2:00 ` Paul Walmsley
1 sibling, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-06 17:33 UTC (permalink / raw)
To: Paul Walmsley, devicetree, linux-clk
Cc: Paul Walmsley, Michael Turquette, Rob Herring, Mark Rutland,
Palmer Dabbelt, Megan Wachs, linux-riscv, linux-kernel,
Paul Walmsley
Quoting Paul Walmsley (2018-10-20 06:50:23)
> 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..d7c1e83fa5ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
> @@ -0,0 +1,43 @@
> +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<version>". As of the time this
> + file was written, only one value is supported:
I don't think we need the "As of the time" sentence. It will become out
of date almost immediately so why not just say "The list of supported
values is:"?
> + "sifive,fu540-c000-prci0"
> +- 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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver
2018-11-06 17:33 ` Stephen Boyd
@ 2018-11-07 2:00 ` Paul Walmsley
0 siblings, 0 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-11-07 2:00 UTC (permalink / raw)
To: Stephen Boyd
Cc: Paul Walmsley, devicetree, linux-clk, Mark Rutland, Megan Wachs,
Michael Turquette, Palmer Dabbelt, linux-kernel, Rob Herring,
linux-riscv, paul
On Tue, 6 Nov 2018, Stephen Boyd wrote:
> Quoting Paul Walmsley (2018-10-20 06:50:23)
> > 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..d7c1e83fa5ed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
> > @@ -0,0 +1,43 @@
> > +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<version>". As of the time this
> > + file was written, only one value is supported:
>
> I don't think we need the "As of the time" sentence. It will become out
> of date almost immediately so why not just say "The list of supported
> values is:"?
Adopted this change. Thanks for the review.
- Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library
2018-10-25 7:47 ` Stephen Boyd
@ 2018-11-09 1:01 ` Paul Walmsley
2018-11-21 8:43 ` Stephen Boyd
0 siblings, 1 reply; 14+ messages in thread
From: Paul Walmsley @ 2018-11-09 1:01 UTC (permalink / raw)
To: Stephen Boyd, devicetree, linux-clk
Cc: Wesley Terpstra, Palmer Dabbelt, Michael Turquette, Megan Wachs,
linux-kernel, Paul Walmsley
Hi Stephen,
On 10/25/18 12:47 AM, Stephen Boyd wrote:
> Quoting Paul Walmsley (2018-10-20 06:50:22)
>> 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
> Weird tab ^?
Thanks. I must be getting senile to have missed that.
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
>> new file mode 100644
>> index 000000000000..ebdef859cbf5
>> --- /dev/null
>> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
>> @@ -0,0 +1,387 @@
>> +// 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.
>> + *
> Can you drop these two paragraphs? It's duplicated from the SPDX tag.
Yep, fixed.
>> + * 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"
> Any html links?
The Analog Bits datasheet is not available on-line, to my knowledge.
The comments in this driver are an attempt to document it for software
developers.
The SiFive datasheet is available online; just added a link to the comments.
>> + */
>> +
>> +#include <linux/bug.h>
> Is this used?
Yes - the library doesn't compile without it:
CC drivers/clk/analogbits/wrpll-cln28hpc.o
drivers/clk/analogbits/wrpll-cln28hpc.c: In function
'__wrpll_calc_filter_range':
drivers/clk/analogbits/wrpll-cln28hpc.c:83:3: error: implicit
declaration of function 'WARN' [-Werror=implicit-function-declaration]
WARN(1, "%s: post-divider reference freq out of range: %lu",
^~~~
(etc.)
>> +#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.
> Would that ever be needed? Presumably the flag isn't changing.
It's just another way of writing that the caller needs to lock the data
pointed to by c against simultaneous modification, freeing, etc.
Ideally this would be obvious to callers by virtue of the pointer being
passed, but figured there was no harm in explicitly stating it. Let me
know if you want me to remove the comment.
>> + *
>> + * 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)
>> + */
>> +static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)
> Nitpick: Why so much underscore in this code?
The intention was to mark file-local functions from global functions (as
you recognize below). Of course, I should have just used one
underscore: just re-reviewed the C standard and turns out I misrecalled
the usual way for doing this - one underscore is appropriate. But, see
below.
>> +{
>> + u64 s;
>> + u8 divq = 0;
>> +
>> + if (!vco_rate) {
>> + WARN_ON(1);
>> + goto wcd_out;
> Just return here, There aren't any locks so no need for goto.
(See below)
>
> Also, don't check anything? The caller is bad if this happens, and this
> is a static function so it can pretty much be found by visual
> inspection or during development?
Agreed this makes sense for the static functions. Dropped & kerneldoc
updated.
>> + }
>> +
>> + 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;
> Does this need to be a u8? It makes the min_t below required instead of
> using an unsigned int.
I've changed this as you request, and added the (subsequently required)
type suffix to the macro value, along with an appropriate comment.
>> +
>> + if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
>> + return -1;
> Please pick some valid error code instead of -1 (I thought checkpatch
> complained about this?).
checkpatch isn't complaining here. Anyway, fixed to return a reasonable
error code.
> Is this case even possible?
If analogbits_wrpll_configure_for_rate() is called with a parent_rate
outside of the PLL's specification range, it seems reasonable to fail --
or do you see the situation differently? These parent rate restrictions
are defined in the PLL datasheet.
> It would be nicer
> if this function just returned void.
Is it that you'd prefer the rate check to be moved into
analogbits_wrpll_configure_for_rate()? Or that you'd prefer that I not
validate the input parent rate at all?
>> +
>> + 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;
>> +}
>> +
>> +/*
>> + * Public functions
>> + */
>> +
>> +/**
>> + * 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, fbcfg;
>> + u8 fbdiv, divq, best_r, r;
>> +
>> + if (!c)
>> + return -1;
> -EINVAL? Of course, it's probably better to just not care and blow up if
> the user of the API passes in NULL.
Agreed for statically-scoped functions it's most likely overkill. But
for globally-scoped functions like this, it seemed better to catch the
errors rather than to blow up. The underlying approach was intended to
be standard precondition-based (paranoia-based?) development. Seems
like I often screw up the calling side during development, so figured
this approach would save human time. The performance impact seems
minimal. That said, if it's your strong preference or requirement for
these to be dropped, I will of course do it - let me know.
In the meantime, have changed the return value to -EINVAL per your comment.
>> +
>> + if (c->flags == 0) {
>> + WARN(1, "%s called with uninitialized PLL config", __func__);
> Do we really need the stacktrace in this case? Or can this be downgraded
> to a pr_warn() and return some error value that isn't -1?
Since this case should never happen, and indicates either a bug in the
code or corrupted memory, the intention was to warn loudly so it was
difficult to be ignore. So I would prefer to keep it annoying. But
again, if it's important, would be OK with changing it - let me know.
Usually I try to be more paranoid with the globally-scoped functions
(like this one) than with the file-local functions.
>> + return -1;
>> + }
>> +
>> + fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
>> + if ((c->flags & fbcfg) == fbcfg) {
>> + WARN(1, "%s called with invalid PLL config", __func__);
>> + return -1;
>> + }
>> +
>> + if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
>> + WARN(1, "%s: external feedback mode not currently supported",
>> + __func__);
>> + return -1;
>> + }
> Like a bunch of this stuff, why are we checking things that the caller
> should know to do itself? I'd prefer we keep things simple and assume
> the caller knows what they're doing.
My thinking was that it's easy to screw up if someone is unfamiliar with
the library. Since the preconditions are computationally simple to
verify, the runtime cost is essentially zero. Put differently, they are
intended to trade off a small amount of CPU time to save a larger amount
of human time.
>> +
>> + /* 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)
> const c?
Done
>> +{
>> + return MAX_LOCK_US;
> And we're planning to change this in the future?
Yes, we're in a discussion with the PLL designer about this, and I
expect it to change within another merge window.
>> +}
>> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
>> new file mode 100644
>> index 000000000000..cc4268f16067
>> --- /dev/null
>> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
>> @@ -0,0 +1,99 @@
>> +/* 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.
> Ah ok. Are the functions also done this way for a private/public API
> split.
Yep.
> We don't typically care about this in the kernel, just mark it
> static if it's private and non-static if it's public for functions.
Used to do it this way a long ago, perhaps a bit less incompetently -
here's an example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-omap2/omap_hwmod.c?h=v4.1#n383
I personally prefer the explicit indication that the identifier was
scoped file-local in the calling functions. But am fine changing it per
your comments, and have done so.
> Underscores just make lines longer.
Hehe.
> And for struct members we have
> kernel doc 'private' tag that can tell users to not do something. Or
> worst case, an opaque pointer is stored and only defined in the C file.
In any case, I've changed the identifier names to align to your comments.
>> + */
>> +struct analogbits_wrpll_cfg {
>> + u8 divr;
>> + u8 divq;
>> + u8 range;
>> + u8 flags;
>> + u16 divf;
>> + u32 _output_rate_cache[DIVQ_VALUES];
>> + unsigned long _parent_rate;
>> + u8 _max_r;
>> + u8 _init_r;
>> +};
>> +
>> +/*
>> + * Function prototypes
>> + */
> Please don't have these types of comments. They don't help.
Dropped this one. Do you want me to nuke the similar comments in
wrpll-cln28hpc.c, separating the private function section from the
public functions, for example?
Thanks for your review,
- Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver
2018-10-24 18:47 ` Rob Herring
@ 2018-11-16 23:18 ` Paul Walmsley
0 siblings, 0 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-11-16 23:18 UTC (permalink / raw)
To: Rob Herring
Cc: Paul Walmsley, linux-clk, devicetree, Michael Turquette,
Stephen Boyd, Mark Rutland, Palmer Dabbelt, Megan Wachs,
linux-riscv, linux-kernel, Paul Walmsley
On Wed, 24 Oct 2018, Rob Herring wrote:
> On Sat, Oct 20, 2018 at 06:50:23AM -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.
>>
>> 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
>> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> ---
>> v2: remove out-of-date example, add documentation for the compatible
>> string and for the required PCB clock nodes
>>
>> .../bindings/clock/sifive/fu540-prci.txt | 43 +++++++++++++++++++
>> 1 file changed, 43 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..d7c1e83fa5ed
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.txt
>> @@ -0,0 +1,43 @@
>> +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<version>". As of the time this
>> + file was written, only one value is supported:
>> + "sifive,fu540-c000-prci0"
>
> What happens with this depends on the discussion on the other bindings.
We'll drop the trailing 0 since the SoC identifier prefix should be
sufficiently precise.
> Though here you are inconsistent without a fallback. Of course, I've
> never seen a clock controller be the same across SoCs.
As you write, the assumption is that chip integration IP blocks like this
one are likely to be specific to individual SoCs.
This may not be universally true for SiFive looking into the future, but
since we don't yet have a clear sense of the extent of exact reuse (i.e.
chip "families"), am not yet comfortable with advocating something like
"sifive,prci0" yet, as we do with the sifive-blocks.
>> +- 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/linux/clk/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.
>> +
>> +Examples:
>> +
>> +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";
>> +};
>> +prci0: prci@10000000 {
>
> clock-controller@...
Thanks; fixed.
- Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library
2018-11-09 1:01 ` Paul Walmsley
@ 2018-11-21 8:43 ` Stephen Boyd
2018-11-27 2:55 ` Paul Walmsley
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-21 8:43 UTC (permalink / raw)
To: Paul Walmsley, devicetree, linux-clk
Cc: Wesley Terpstra, Palmer Dabbelt, Michael Turquette, Megan Wachs,
linux-kernel, Paul Walmsley
Quoting Paul Walmsley (2018-11-08 17:01:54)
> On 10/25/18 12:47 AM, Stephen Boyd wrote:
> > Quoting Paul Walmsley (2018-10-20 06:50:22)
> >> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
> >> new file mode 100644
> >> index 000000000000..ebdef859cbf5
> >> --- /dev/null
> >> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
> >> @@ -0,0 +1,387 @@
> >> +// 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.
> >> + *
> > Can you drop these two paragraphs? It's duplicated from the SPDX tag.
>
>
> Yep, fixed.
>
>
> >> + * 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"
> > Any html links?
>
>
> The Analog Bits datasheet is not available on-line, to my knowledge.
> The comments in this driver are an attempt to document it for software
> developers.
>
>
> The SiFive datasheet is available online; just added a link to the comments.
Ok.
>
>
> >> + */
> >> +
> >> +#include <linux/bug.h>
> > Is this used?
>
>
> Yes - the library doesn't compile without it:
Alright.
>
>
> >> +#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.
> > Would that ever be needed? Presumably the flag isn't changing.
>
>
> It's just another way of writing that the caller needs to lock the data
> pointed to by c against simultaneous modification, freeing, etc.
> Ideally this would be obvious to callers by virtue of the pointer being
> passed, but figured there was no harm in explicitly stating it. Let me
> know if you want me to remove the comment.
Hmm ok. Doesn't hurt so no need to change.
>
>
> >> + }
> >> +
> >> + 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;
> > Does this need to be a u8? It makes the min_t below required instead of
> > using an unsigned int.
>
>
> I've changed this as you request, and added the (subsequently required)
> type suffix to the macro value, along with an appropriate comment.
>
>
> >> +
> >> + if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
> >> + return -1;
> > Please pick some valid error code instead of -1 (I thought checkpatch
> > complained about this?).
>
>
> checkpatch isn't complaining here. Anyway, fixed to return a reasonable
> error code.
Ok. I must be remembering some never upstreamed checkpatch patch, ouch!
>
>
> > Is this case even possible?
>
>
> If analogbits_wrpll_configure_for_rate() is called with a parent_rate
> outside of the PLL's specification range, it seems reasonable to fail --
> or do you see the situation differently? These parent rate restrictions
> are defined in the PLL datasheet.
I've been pushing driver writers to use clk_hw_set_rate_range() so that
the core framework will clamp the frequency to what is supported. Can
you somehow use that for these clks?
>
>
> > It would be nicer
> > if this function just returned void.
>
>
> Is it that you'd prefer the rate check to be moved into
> analogbits_wrpll_configure_for_rate()? Or that you'd prefer that I not
> validate the input parent rate at all?
I lost the context! But I think I'm just saying that if the clamping is
done in the core framework with limits then this function can just
return void because there aren't errors to be had.
>
>
> >> +
> >> + 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;
> >> +}
> >> +
> >> +/*
> >> + * Public functions
> >> + */
> >> +
> >> +/**
> >> + * 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, fbcfg;
> >> + u8 fbdiv, divq, best_r, r;
> >> +
> >> + if (!c)
> >> + return -1;
> > -EINVAL? Of course, it's probably better to just not care and blow up if
> > the user of the API passes in NULL.
>
>
> Agreed for statically-scoped functions it's most likely overkill. But
> for globally-scoped functions like this, it seemed better to catch the
> errors rather than to blow up. The underlying approach was intended to
> be standard precondition-based (paranoia-based?) development. Seems
> like I often screw up the calling side during development, so figured
> this approach would save human time. The performance impact seems
> minimal. That said, if it's your strong preference or requirement for
> these to be dropped, I will of course do it - let me know.
I would still prefer to remove it. It's a semi-global API that's really
only used and exported by clk providers. We should expect an oops and
big callstack/crash if some driver author is using the API improperly.
Hopefully those developers are testing their code, and they see that
it's experiencing problems here. So the calling side screwing up during
development will very quickly realize they're doing something wrong vs.
noticing later that some clk is not there.
>
>
> In the meantime, have changed the return value to -EINVAL per your comment.
Ok!
>
>
> >> +
> >> + if (c->flags == 0) {
> >> + WARN(1, "%s called with uninitialized PLL config", __func__);
> > Do we really need the stacktrace in this case? Or can this be downgraded
> > to a pr_warn() and return some error value that isn't -1?
>
>
> Since this case should never happen, and indicates either a bug in the
> code or corrupted memory, the intention was to warn loudly so it was
> difficult to be ignore. So I would prefer to keep it annoying. But
> again, if it's important, would be OK with changing it - let me know.
> Usually I try to be more paranoid with the globally-scoped functions
> (like this one) than with the file-local functions.
>
>
> >> + return -1;
> >> + }
> >> +
> >> + fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
> >> + if ((c->flags & fbcfg) == fbcfg) {
> >> + WARN(1, "%s called with invalid PLL config", __func__);
> >> + return -1;
> >> + }
> >> +
> >> + if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
> >> + WARN(1, "%s: external feedback mode not currently supported",
> >> + __func__);
> >> + return -1;
> >> + }
> > Like a bunch of this stuff, why are we checking things that the caller
> > should know to do itself? I'd prefer we keep things simple and assume
> > the caller knows what they're doing.
>
>
> My thinking was that it's easy to screw up if someone is unfamiliar with
> the library. Since the preconditions are computationally simple to
> verify, the runtime cost is essentially zero. Put differently, they are
> intended to trade off a small amount of CPU time to save a larger amount
> of human time.
>
Can we move the checks to build time with some macro that defines the
configuration structure and has BUILD_BUG_ON() checks? Sure it doesn't
have much runtime overhead, but it has a codesize overhead and mental
logic overhead for something that would be better served with some
static check or documentation/comments and code review. Put another way,
it's not like this configuration is coming from random user input and so
we need to validate the input to make sure it's sane. We can validate
the input when merging the code or when building the code and avoid this
all.
>
>
> >> +}
> >> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> >> new file mode 100644
> >> index 000000000000..cc4268f16067
> >> --- /dev/null
> >> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> >> +
> >> +/**
> >> + * 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.
Drop the full-stops on here too please.
> >> + * @_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.
> > Ah ok. Are the functions also done this way for a private/public API
> > split.
>
>
> Yep.
Ok.
>
>
> > We don't typically care about this in the kernel, just mark it
> > static if it's private and non-static if it's public for functions.
>
>
> Used to do it this way a long ago, perhaps a bit less incompetently -
> here's an example:
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-omap2/omap_hwmod.c?h=v4.1#n383
>
>
> I personally prefer the explicit indication that the identifier was
> scoped file-local in the calling functions. But am fine changing it per
> your comments, and have done so.
Ok. I haven't really seen the underscores, except for in the cases when
some framework API needs to be split into some backend multi-plexing
function to handle different options while keeping the original function
signature around. Put another way, it's not a kernel idiom for driver
code, but I'm not too worried about it, so go with your own style if it
makes you happy.
>
>
> > And for struct members we have
> > kernel doc 'private' tag that can tell users to not do something. Or
> > worst case, an opaque pointer is stored and only defined in the C file.
>
>
> In any case, I've changed the identifier names to align to your comments.
Ok. Please also use the private tag to indicate that these are internal
things that shouldn't be used.
>
>
> >> + */
> >> +struct analogbits_wrpll_cfg {
> >> + u8 divr;
> >> + u8 divq;
> >> + u8 range;
> >> + u8 flags;
> >> + u16 divf;
> >> + u32 _output_rate_cache[DIVQ_VALUES];
> >> + unsigned long _parent_rate;
> >> + u8 _max_r;
> >> + u8 _init_r;
> >> +};
> >> +
> >> +/*
> >> + * Function prototypes
> >> + */
> > Please don't have these types of comments. They don't help.
>
>
> Dropped this one. Do you want me to nuke the similar comments in
> wrpll-cln28hpc.c, separating the private function section from the
> public functions, for example?
>
Yes.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block
2018-10-20 13:50 ` [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block Paul Walmsley
@ 2018-11-21 16:35 ` Stephen Boyd
2018-11-27 23:24 ` Paul Walmsley
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-21 16:35 UTC (permalink / raw)
To: Paul Walmsley, devicetree, linux-clk
Cc: Paul Walmsley, Michael Turquette, Albert Ou, Wesley W . Terpstra,
Palmer Dabbelt, Megan Wachs, linux-riscv, linux-kernel,
Paul Walmsley
Quoting Paul Walmsley (2018-10-20 06:50:24)
> diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> new file mode 100644
> index 000000000000..870cb8333648
> --- /dev/null
> +++ b/drivers/clk/sifive/fu540-prci.c
> @@ -0,0 +1,634 @@
> +// SPDX-License-Identifier: GPL-2.0
[...]
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
Is this for of_clk_get_parent_count()? Use of_clk.h include for that,
and drop this clk.h include if possible.
> +#include <linux/clk/analogbits-wrpll-cln28hpc.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/log2.h>
Is this used?
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/clk/sifive-fu540-prci.h>
Can you sort these at least by path includes so that linux/clk/ is
somewhere together?
> +
> +/*
> + * 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;
Usually this is called 'base', but 'va' is fine too. What happens with
NOMMU? Then it's not a VA anymore?
> + struct clk_hw_onecell_data hw_clks;
> +};
> +
> +/**
> + * struct __prci_wrpll_data - WRPLL configuration and integration data
> + * @c: WRPLL current configuration record
> + * @bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
> + * @no_bypass: fn ptr to code to not bypass the WRPLL (if applicable; else NULL)
> + * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
> + *
> + * @bypass and @no_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 (*bypass)(struct __prci_data *pd);
> + void (*no_bypass)(struct __prci_data *pd);
> + u8 cfg0_offs;
> +};
Why do we have this struct? Why not fold it into the prci_clock
structure? As far as I can tell it's a one to one correlation right now.
> +
> +/**
> + * 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);
> +}
Please remove these wrappers. The lines are only barely shorter with
them, they mostly obfuscate the code, and writel swaps 'offs' and 'pd'
vs readl (why?).
__prci_wrpll_unpack(&pwd->c, __prci_readl(pd, pwd->cfg0_offs));
vs.
__prci_wrpll_unpack(&pwd->c, readl_relaxed(pd->va + pwd->cfg0_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 >>= 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);
> +
> + if (r & PRCI_COREPLLCFG0_FSE_MASK)
> + c->flags |= WRPLL_FLAGS_INT_FEEDBACK_MASK;
> + else
> + c->flags |= WRPLL_FLAGS_EXT_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 *c)
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;
> + if (c->flags & WRPLL_FLAGS_INT_FEEDBACK_MASK)
> + 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_unpack(&pwd->c, readl_relaxed(pd->va + 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));
I'm trying to understand what all the memcpy() calls in this driver are
doing. What is being optimized by storing the values in software? Is I/O
access really that bad that we need to use memcpy()? Can you remove it
all and just read and write registers when we need them? It would make
things clearer and shorter. If you need caching, I would suggest you use
regmap and all the caching features therein.
> +}
> +
> +/* 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 */
Barrier with what? What are we synchronizing with?
> +}
> +
> +/**
> + * __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);
I suppose this is one place where the caching must have gone wrong, so
then we needed to add __prci_wrpll_read_cfg() to make sure we've kept
things in sync with our memcpy? Please don't do that. Just read the
hardware in recalc_rate() so that we don't have to worry about syncing
framework state with this driver state.
> +}
> +
> +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 -ERANGE;
Why not return the value of 'r'?
> +
> + if (pwd->bypass)
> + pwd->bypass(pd);
> +
> + __prci_wrpll_write_cfg(pd, pwd, &pwd->c);
> +
> + udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));
> +
> + if (pwd->no_bypass)
> + pwd->no_bypass(pd);
Maybe call it enable/disable_bypass() instead? no_bypass sounds like
it's doing something if there isn't a bypass.
> +
> + return 0;
> +}
[...]
> +
> +/**
> + * __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 Linux error code upon failure.
This is all Linux, so drop the "Linux" part?
> + */
> +static int __prci_register_clocks(struct device *dev)
> +{
> + struct __prci_data *pd = dev_get_drvdata(dev);
> + 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 two parent clocks, only found %d\n",
> + parent_count);
Heh, this would read funny if it says "expected two parent clocks, only
found 50". Can this be enforced with DT schema instead of checking at
runtime in the driver? We don't typically validate DT in the kernel
because the kernel is not a DT validator.
> + 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 = (const char *[]) { pic->parent_name };
Just use &pic->parent_name instead?
> + init.num_parents = 1;
> + init.ops = pic->ops;
> + init.flags = CLK_IGNORE_UNUSED;
Why? Can you remove it? Or add a comment indicating why you need to have
it here?
> + 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));
Do you need clkdev? I would avoid clkdev if possible and just use DT
based lookups until you need clkdev.
> + 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 -EINVAL;
Why override error code?
> + }
> +
> + 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(struct __prci_data), GFP_KERNEL);
sizeof(*pd) please, makes shorter lines and avoids types changing.
> + if (!pd)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, pd);
Why not just pass the pd to __prci_register_clocks() and not have any
driver data?
> +
> + 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);
Cool!
> +
> + r = __prci_register_clocks(dev);
> + if (r) {
> + dev_err(dev, "could not register clocks: %d\n", r);
> + return -EINVAL;
But we override return value and always return -EINVAL? Why not return
r?
> + }
> +
> + dev_info(dev, "SiFive FU540 PRCI probed\n");
Please no "I'm alive!" messages.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sifive_fu540_prci_of_match[] = {
> + { .compatible = "sifive,fu540-c000-prci0", },
> + {}
> +};
Can it be a module? If so, add an MODULE_DEVICE_TABLE(of, ...) here.
> +
> +static struct platform_driver sifive_fu540_prci_driver = {
> + .driver = {
> + .name = "sifive-fu540-prci",
> + .of_match_table = sifive_fu540_prci_of_match,
And consider suppressing unbind from sysfs here if you don't want that
feature.
> + },
> + .probe = sifive_fu540_prci_probe,
Especially because there isn't a remove function.
> +};
> +
> +static int __init sifive_fu540_prci_init(void)
> +{
> + return platform_driver_register(&sifive_fu540_prci_driver);
> +}
> +core_initcall(sifive_fu540_prci_init);
> diff --git a/include/linux/clk/sifive-fu540-prci.h b/include/linux/clk/sifive-fu540-prci.h
> new file mode 100644
> index 000000000000..5d03eae7d4ef
> --- /dev/null
> +++ b/include/linux/clk/sifive-fu540-prci.h
> @@ -0,0 +1,27 @@
> +/* 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.
Same SPDX comments apply here.
> + */
> +
> +#ifndef __LINUX_CLK_SIFIVE_FU540_PRCI_H
> +#define __LINUX_CLK_SIFIVE_FU540_PRCI_H
> +
> +/* Clock indexes for use by Device Tree data */
Seems obvious, but OK!
> +
> +#define PRCI_CLK_COREPLL 0
> +#define PRCI_CLK_DDRPLL 1
> +#define PRCI_CLK_GEMGXLPLL 2
> +#define PRCI_CLK_TLCLK 3
> +
> +#endif
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library
2018-11-21 8:43 ` Stephen Boyd
@ 2018-11-27 2:55 ` Paul Walmsley
0 siblings, 0 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-11-27 2:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: Paul Walmsley, devicetree, linux-clk, Wesley Terpstra,
Palmer Dabbelt, Michael Turquette, Megan Wachs, linux-kernel,
Paul Walmsley
[-- Attachment #1: Type: text/plain, Size: 12481 bytes --]
On Wed, 21 Nov 2018, Stephen Boyd wrote:
> Quoting Paul Walmsley (2018-11-08 17:01:54)
> > On 10/25/18 12:47 AM, Stephen Boyd wrote:
> > > Quoting Paul Walmsley (2018-10-20 06:50:22)
> > >> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
> > >> new file mode 100644
> > >> index 000000000000..ebdef859cbf5
> > >> --- /dev/null
> > >> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
[...]
> > >> +/**
> > >> + * __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;
[...]
> >
> > > Is this case even possible?
> >
> > If analogbits_wrpll_configure_for_rate() is called with a parent_rate
> > outside of the PLL's specification range, it seems reasonable to fail --
> > or do you see the situation differently? These parent rate restrictions
> > are defined in the PLL datasheet.
>
> I've been pushing driver writers to use clk_hw_set_rate_range() so that
> the core framework will clamp the frequency to what is supported. Can
> you somehow use that for these clks?
Although the code in this patch is related to a hardware clock source (the
WRPLL), this code isn't dependent on the Linux clock framework. It's just
a library that implements algorithms that are specific to this PLL.
Patch 3 ("clk: sifive: add a driver for the SiFive FU540 PRCI IP block")
adds drivers/clk/sifive/fu540-prci.c, which integrates the library with
the CCF.
We could also make the code in this patch dependent on the Linux clock
framework. My thought was that CCF dependencies weren't really needed in
this file, and that the algorithms would be easier to read and analyze
without them.
So if it's all right with you, I'd propose that we keep clock
framework-dependent functions localized into
drivers/clk/sifive/fu540-prci.c.
Let me know what you'd like.
> > > It would be nicer
> > > if this function just returned void.
> >
> >
> > Is it that you'd prefer the rate check to be moved into
> > analogbits_wrpll_configure_for_rate()? Or that you'd prefer that I not
> > validate the input parent rate at all?
>
> I lost the context! But I think I'm just saying that if the clamping is
> done in the core framework with limits then this function can just
> return void because there aren't errors to be had.
OK. The outcome here seems connected with the discussion outcome on
the above comment, since this code is written to avoid adding clock
framework dependencies. So will wait for your thoughts on that before
taking action here.
> > >> +/*
> > >> + * Public functions
> > >> + */
> > >> +
> > >> +/**
> > >> + * 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, fbcfg;
> > >> + u8 fbdiv, divq, best_r, r;
> > >> +
> > >> + if (!c)
> > >> + return -1;
> > > -EINVAL? Of course, it's probably better to just not care and blow up if
> > > the user of the API passes in NULL.
> >
> >
> > Agreed for statically-scoped functions it's most likely overkill. But
> > for globally-scoped functions like this, it seemed better to catch the
> > errors rather than to blow up. The underlying approach was intended to
> > be standard precondition-based (paranoia-based?) development. Seems
> > like I often screw up the calling side during development, so figured
> > this approach would save human time. The performance impact seems
> > minimal. That said, if it's your strong preference or requirement for
> > these to be dropped, I will of course do it - let me know.
>
> I would still prefer to remove it. It's a semi-global API that's really
> only used and exported by clk providers. We should expect an oops and
> big callstack/crash if some driver author is using the API improperly.
> Hopefully those developers are testing their code, and they see that
> it's experiencing problems here. So the calling side screwing up during
> development will very quickly realize they're doing something wrong vs.
> noticing later that some clk is not there.
OK - dropped the null pointer check.
> > >> + return -1;
> > >> + }
> > >> +
> > >> + fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
> > >> + if ((c->flags & fbcfg) == fbcfg) {
> > >> + WARN(1, "%s called with invalid PLL config", __func__);
> > >> + return -1;
> > >> + }
> > >> +
> > >> + if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
> > >> + WARN(1, "%s: external feedback mode not currently supported",
> > >> + __func__);
> > >> + return -1;
> > >> + }
> > > Like a bunch of this stuff, why are we checking things that the caller
> > > should know to do itself? I'd prefer we keep things simple and assume
> > > the caller knows what they're doing.
> >
> > My thinking was that it's easy to screw up if someone is unfamiliar with
> > the library. Since the preconditions are computationally simple to
> > verify, the runtime cost is essentially zero. Put differently, they are
> > intended to trade off a small amount of CPU time to save a larger amount
> > of human time.
>
> Can we move the checks to build time with some macro that defines the
> configuration structure and has BUILD_BUG_ON() checks?
In the short term, I agree that it's unlikely that PLLs will be
instantiated with this library using anything other than static C
structure records. In the future, these PLLs may be instantiated
completely dynamically from data not available at build time, at which
point the static checks won't catch problems there.
> Sure it doesn't have much runtime overhead, but it has a codesize
> overhead and mental logic overhead for something that would be better
> served with some static check or documentation/comments and code review.
> Put another way, it's not like this configuration is coming from random
> user input and so we need to validate the input to make sure it's sane.
> We can validate the input when merging the code or when building the
> code and avoid this all.
Seems like we have similar goals in mind; just different ways of viewing
the problem. I personally rest easier knowing that the preconditions are
always satisfied before entering the function. That way there's no need
to worry about whether a given library user (which may not be the Linux
clock framework) really is careful about what's being passed in. Another
nice side effect about verifying parameter domains at runtime is that it
can reduce the amount of anguish involved in debugging runtime memory
corruption bugs, since the preconditions can catch corruption closer to
when it actually happens.
In any case - I will post a revision with them implemented statically, as
you request.
> > >> +}
> > >> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> > >> new file mode 100644
> > >> index 000000000000..cc4268f16067
> > >> --- /dev/null
> > >> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> > >> +
> > >> +/**
> > >> + * 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.
>
> Drop the full-stops on here too please.
Done, thanks.
> > > We don't typically care about this in the kernel, just mark it
> > > static if it's private and non-static if it's public for functions.
> >
> > Used to do it this way a long ago, perhaps a bit less incompetently -
> > here's an example:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-omap2/omap_hwmod.c?h=v4.1#n383
> >
> > I personally prefer the explicit indication that the identifier was
> > scoped file-local in the calling functions. But am fine changing it per
> > your comments, and have done so.
>
> Ok. I haven't really seen the underscores, except for in the cases when
> some framework API needs to be split into some backend multi-plexing
> function to handle different options while keeping the original function
> signature around. Put another way, it's not a kernel idiom for driver
> code, but I'm not too worried about it, so go with your own style if it
> makes you happy.
They've been dropped already from this patch based on our earlier
discussion, so will just keep them dropped.
> > > And for struct members we have
> > > kernel doc 'private' tag that can tell users to not do something. Or
> > > worst case, an opaque pointer is stored and only defined in the C file.
> >
> >
> > In any case, I've changed the identifier names to align to your comments.
>
> Ok. Please also use the private tag to indicate that these are internal
> things that shouldn't be used.
Done.
> > >> + */
> > >> +struct analogbits_wrpll_cfg {
> > >> + u8 divr;
> > >> + u8 divq;
> > >> + u8 range;
> > >> + u8 flags;
> > >> + u16 divf;
> > >> + u32 _output_rate_cache[DIVQ_VALUES];
> > >> + unsigned long _parent_rate;
> > >> + u8 _max_r;
> > >> + u8 _init_r;
> > >> +};
> > >> +
> > >> +/*
> > >> + * Function prototypes
> > >> + */
> > > Please don't have these types of comments. They don't help.
> >
> > Dropped this one. Do you want me to nuke the similar comments in
> > wrpll-cln28hpc.c, separating the private function section from the
> > public functions, for example?
>
> Yes.
Done.
Thanks for the review,
- Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block
2018-11-21 16:35 ` Stephen Boyd
@ 2018-11-27 23:24 ` Paul Walmsley
0 siblings, 0 replies; 14+ messages in thread
From: Paul Walmsley @ 2018-11-27 23:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: Paul Walmsley, devicetree, linux-clk, Michael Turquette,
Albert Ou, Wesley W. Terpstra, Palmer Dabbelt, Megan Wachs,
linux-riscv, linux-kernel, Paul Walmsley
On Wed, 21 Nov 2018, Stephen Boyd wrote:
> Quoting Paul Walmsley (2018-10-20 06:50:24)
> > diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> > new file mode 100644
> > index 000000000000..870cb8333648
> > --- /dev/null
> > +++ b/drivers/clk/sifive/fu540-prci.c
> > @@ -0,0 +1,634 @@
> > +// SPDX-License-Identifier: GPL-2.0
> [...]
> > +
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clk.h>
>
> Is this for of_clk_get_parent_count()? Use of_clk.h include for that,
> and drop this clk.h include if possible.
Done.
> > +#include <linux/clk/analogbits-wrpll-cln28hpc.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/log2.h>
>
> Is this used?
It isn't. Dropped.
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk/sifive-fu540-prci.h>
>
> Can you sort these at least by path includes so that linux/clk/ is
> somewhere together?
Done.
> > +/**
> > + * 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;
>
> Usually this is called 'base', but 'va' is fine too.
OK
> What happens with NOMMU? Then it's not a VA anymore?
It's similar to other Linux code that refers to virtual addresses:
$ fgrep -r va linux/ | fgrep __iomem | egrep -iv '(val|riva)' | wc -l
270
$
(as a first-order approximation)
> > +/**
> > + * struct __prci_wrpll_data - WRPLL configuration and integration data
> > + * @c: WRPLL current configuration record
> > + * @bypass: fn ptr to code to bypass the WRPLL (if applicable; else NULL)
> > + * @no_bypass: fn ptr to code to not bypass the WRPLL (if applicable; else NULL)
> > + * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
> > + *
> > + * @bypass and @no_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 (*bypass)(struct __prci_data *pd);
> > + void (*no_bypass)(struct __prci_data *pd);
> > + u8 cfg0_offs;
> > +};
>
> Why do we have this struct? Why not fold it into the prci_clock
> structure?
The code was written to be extensible to non-PLL clock tree elements.
> As far as I can tell it's a one to one correlation right now.
Certainly true at the moment. Do you want them combined?
> > +/**
> > + * __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);
> > +}
>
> Please remove these wrappers. The lines are only barely shorter with
> them, they mostly obfuscate the code,
Isn't the use of wrappers fairly common CCF and Linux device driver
practice?
$ pcregrep -r \\wwritel drivers/clk | fgrep -v __raw | fgrep -v clk_writel | wc -l
148
$ pcregrep --buffer-size 65536 -r \\wwritel drivers/ | fgrep -v __raw | wc -l
8526
$
The usual motivation is not so much to shorten lines, but to have a
centralized place for adding IP block-specific I/O barriers, register
logging, and other debug.
> and writel swaps 'offs' and 'pd' vs readl (why?).
Yeah. The include/asm-generic/io.h write*() functions invert the argument
order from the read*() argument order. There doesn't appear to be a
strong idiom for device driver write*() argument order in the kernel, so I
just picked one when I wrote them.
> __prci_wrpll_unpack(&pwd->c, __prci_readl(pd, pwd->cfg0_offs));
>
> vs.
>
> __prci_wrpll_unpack(&pwd->c, readl_relaxed(pd->va + pwd->cfg0_offs));
In any case, I am fine with removing them if that is the standard practice
going forward for drivers/clk.
> > +/**
> > + * __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 *c)
>
> const c?
Changed.
> > +/**
> > + * __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));
>
> I'm trying to understand what all the memcpy() calls in this driver are
> doing. What is being optimized by storing the values in software? Is I/O
> access really that bad that we need to use memcpy()? Can you remove it
> all and just read and write registers when we need them? It would make
> things clearer and shorter. If you need caching, I would suggest you use
> regmap and all the caching features therein.
The primary goal is to minimize the number of CPU instructions executed by
this driver when the CPU is running at a low clock rate. This is
important on the FU540 since the PLL bypass clock is slow. Switching to
regmap wouldn't help here, since the driver would still need to do the
computation.
As a secondary goal, the shadow copy of the PLL configuration also
allows the driver to avoid unnecessary PRCI register reads.
> > +/**
> > + * __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 */
>
> Barrier with what?
That's an I/O barrier.
> What are we synchronizing with?
The completion of the preceding write to the PRCI.
> > +/*
> > + * 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);
>
> I suppose this is one place where the caching must have gone wrong, so
> then we needed to add __prci_wrpll_read_cfg() to make sure we've kept
> things in sync with our memcpy? Please don't do that. Just read the
> hardware in recalc_rate() so that we don't have to worry about syncing
> framework state with this driver state.
The code is written to the usual Linux principle that the driver has
exclusive access to the PRCI IP block. Thus the shadow copy should be in
sync with the hardware at all times. The only time that the PLL
configuration needs to be read from the hardware is when the driver
initializes.
Of course, the code could be changed to read the hardware directly. I'd be
curious to know what advantage you see in making that change. MMIO reads
from a leaf IP block are slow, and the CPU cores on this chip are
in-order. The PRCI driver already maintains an up-to-date shadow copy of
the PLL data to avoid unnecessary computation (as mentioned above). Thus
since the shadow copy is already there, it makes sense to use it. But
perhaps I am misunderstanding your comment?
> > +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 -ERANGE;
>
> Why not return the value of 'r'?
Changed.
> > +
> > + if (pwd->bypass)
> > + pwd->bypass(pd);
> > +
> > + __prci_wrpll_write_cfg(pd, pwd, &pwd->c);
> > +
> > + udelay(analogbits_wrpll_calc_max_lock_us(&pwd->c));
> > +
> > + if (pwd->no_bypass)
> > + pwd->no_bypass(pd);
>
> Maybe call it enable/disable_bypass() instead? no_bypass sounds like
> it's doing something if there isn't a bypass.
Changed.
> > +
> > + return 0;
> > +}
> [...]
> > +
> > +/**
> > + * __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 Linux error code upon failure.
>
> This is all Linux, so drop the "Linux" part?
Done.
> > + */
> > +static int __prci_register_clocks(struct device *dev)
> > +{
> > + struct __prci_data *pd = dev_get_drvdata(dev);
> > + 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 two parent clocks, only found %d\n",
> > + parent_count);
>
> Heh, this would read funny if it says "expected two parent clocks, only
> found 50".
Thanks, I've moved the "only" to appear earlier in the message.
> Can this be enforced with DT schema instead of checking at runtime in
> the driver? We don't typically validate DT in the kernel because the
> kernel is not a DT validator.
Doesn't most code that calls of_*() functions in the kernel check the
result? Looks like it based on a brief glance through the output of:
$ fgrep -rA 3 =\ of_ linux/
> > + 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 = (const char *[]) { pic->parent_name };
>
> Just use &pic->parent_name instead?
Done.
> > + init.num_parents = 1;
> > + init.ops = pic->ops;
> > + init.flags = CLK_IGNORE_UNUSED;
>
> Why? Can you remove it? Or add a comment indicating why you need to have
> it here?
It's mostly prophylactic until the device driver code for this SoC
matures. It should be possible to remove it.
> > + 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));
>
> Do you need clkdev? I would avoid clkdev if possible and just use DT
> based lookups until you need clkdev.
For debugging and sysfs code external to this patch, it's pleasant to be
able to use clk_get_sys() et al. Does the use of clkdev cause problems?
> > + 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 -EINVAL;
>
> Why override error code?
Changed.
> > + }
> > +
> > + 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(struct __prci_data), GFP_KERNEL);
>
> sizeof(*pd) please, makes shorter lines and avoids types changing.
Changed.
> > + if (!pd)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, pd);
>
> Why not just pass the pd to __prci_register_clocks() and not have any
> driver data?
Changed.
> > +
> > + r = __prci_register_clocks(dev);
> > + if (r) {
> > + dev_err(dev, "could not register clocks: %d\n", r);
> > + return -EINVAL;
>
> But we override return value and always return -EINVAL? Why not return
> r?
Changed.
> > + }
> > +
> > + dev_info(dev, "SiFive FU540 PRCI probed\n");
>
> Please no "I'm alive!" messages.
Converted this to a dev_dbg(), since I still find it useful during debug.
Let me know if that's not enough.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sifive_fu540_prci_of_match[] = {
> > + { .compatible = "sifive,fu540-c000-prci0", },
> > + {}
> > +};
>
> Can it be a module? If so, add an MODULE_DEVICE_TABLE(of, ...) here.
Added.
> > +
> > +static struct platform_driver sifive_fu540_prci_driver = {
> > + .driver = {
> > + .name = "sifive-fu540-prci",
> > + .of_match_table = sifive_fu540_prci_of_match,
>
> And consider suppressing unbind from sysfs here if you don't want that
> feature.
I'm OK with someone trying to unbind it.
> > + },
> > + .probe = sifive_fu540_prci_probe,
>
> Especially because there isn't a remove function.
Added one, based on other remove functions in drivers/clk.
> > diff --git a/include/linux/clk/sifive-fu540-prci.h b/include/linux/clk/sifive-fu540-prci.h
> > new file mode 100644
> > index 000000000000..5d03eae7d4ef
> > --- /dev/null
> > +++ b/include/linux/clk/sifive-fu540-prci.h
> > @@ -0,0 +1,27 @@
> > +/* 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.
>
> Same SPDX comments apply here.
Done.
- Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-11-27 23:24 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-20 13:50 [PATCH v2 0/3] clk: add driver for the SiFive FU540 PRCI and PLLs it controls Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library Paul Walmsley
2018-10-25 7:47 ` Stephen Boyd
2018-11-09 1:01 ` Paul Walmsley
2018-11-21 8:43 ` Stephen Boyd
2018-11-27 2:55 ` Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 2/3] dt-bindings: clk: add documentation for the SiFive PRCI driver Paul Walmsley
2018-10-24 18:47 ` Rob Herring
2018-11-16 23:18 ` Paul Walmsley
2018-11-06 17:33 ` Stephen Boyd
2018-11-07 2:00 ` Paul Walmsley
2018-10-20 13:50 ` [PATCH v2 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block Paul Walmsley
2018-11-21 16:35 ` Stephen Boyd
2018-11-27 23:24 ` Paul Walmsley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).