[PATCH 0/2] Add support for Renesas PhiClock 9FGV1006 clock generator

[PATCH 0/2] Add support for Renesas PhiClock 9FGV1006 clock generator

[Alex Helms]

Device tree bindings and a common clock framework device driver for
the Renesas PhiClock 9FGV1006 clock generator.

Alex Helms (2):
  dtbindings: clock: Add bindings for Renesas PhiClock
  clk: Add support for Renesas PhiClock clock generator

 .../bindings/clock/renesas,phiclock.yaml      |  81 ++
 MAINTAINERS                                   |   6 +
 drivers/clk/Kconfig                           |   9 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-phiclock.c                    | 729 ++++++++++++++++++
 5 files changed, 826 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
 create mode 100644 drivers/clk/clk-phiclock.c


base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
-- 
2.25.1

[PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Alex Helms]

Add dt bindings for the Renesas PhiClock clock generator.

Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
---
 .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml

diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
new file mode 100644
index 000000000..2b36534d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,phiclock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas PhiClock Clock Generator Device Tree Bindings
+
+maintainers:
+  - Alex Helms <alexander.helms.jy@renesas.com>
+
+description: |
+  The Renesas PhiClock is a programmable I2C clock generator that provides
+  1 reference output and 2 clock outputs.
+
+  The driver supports spread spectrum but only if all configurations use the
+  same spread spectrum parameters. If your configuration uses spread spectrum,
+  you must include renesas,ss-amount-percent, renesas,ss-modulation-hz, and
+  renesas,ss-direction in the device tree.
+
+properties:
+  '#clock-cells':
+    const: 1
+
+  clock-names:
+    items:
+      - const: xin-clkin
+
+  clocks:
+    const: 1
+
+  compatible:
+    enum:
+      - renesas,9fgv1006
+
+  reg:
+    maxItems: 1
+
+  renesas,ss-amount-percent:
+    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
+    minimum: 0
+    maximum: 500
+
+  renesas,ss-modulation-hz:
+    description: Spread spectrum modulation rate in Hz
+    minimum: 30000
+    maximum: 63000
+
+  renesas,ss-direction:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Spread spectrum direction
+    enum: [ down, center ]
+
+required:
+  - clock-names
+  - '#clock-cells'
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    ref25: ref25m {
+      compatible = "fixed-clock";
+      #clock-cells = <0>;
+      clock-frequency = <25000000>;
+    };
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      phiclock: clock-controller@68 {
+        compatible = "renesas,9fgv1006";
+        reg = <0x68>;
+        #clock-cells = <1>;
+        clocks = <&ref25>;
+        clock-names = "xin-clkin";
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 256f03904..7eabe930b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17641,6 +17641,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/clock/renesas,versaclock7.yaml
 F:	drivers/clk/clk-versaclock7.c
 
+RENESAS PHICLOCK CLOCK DRIVER
+M:	Alex Helms <alexander.helms.jy@renesas.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
+
 RESET CONTROLLER FRAMEWORK
 M:	Philipp Zabel <p.zabel@pengutronix.de>
 S:	Maintained
-- 
2.25.1

[PATCH 2/2] clk: Add support for Renesas PhiClock clock generator

[Alex Helms]

Add clock device driver for Renesas PhiClock clock generator. The device
takes 1 input clock and has 1 ref out and 2 clock outputs that are the
same frequency. This driver is for the 9FGV1006 device. It supports
changing the frequency with or without spread spectrum enabled, and gating
each output clock.

Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
---
 MAINTAINERS                |   1 +
 drivers/clk/Kconfig        |   9 +
 drivers/clk/Makefile       |   1 +
 drivers/clk/clk-phiclock.c | 729 +++++++++++++++++++++++++++++++++++++
 4 files changed, 740 insertions(+)
 create mode 100644 drivers/clk/clk-phiclock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7eabe930b..0a8b429e1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17645,6 +17645,7 @@ RENESAS PHICLOCK CLOCK DRIVER
 M:	Alex Helms <alexander.helms.jy@renesas.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
+F:	drivers/clk/clk-phiclock.c
 
 RESET CONTROLLER FRAMEWORK
 M:	Philipp Zabel <p.zabel@pengutronix.de>
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index d79905f3e..6bcdde0f0 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -386,6 +386,15 @@ config COMMON_CLK_VC7
 	  Renesas Versaclock7 is a family of configurable clock generator
 	  and jitter attenuator ICs with fractional and integer dividers.
 
+config COMMON_CLK_PHICLOCK
+	tristate "Clock driver for Renesas PhiClock devices"
+	depends on I2C
+	depends on OF
+	select REGMAP_I2C
+	help
+	  Renesas PhiClock is a clock generator with a fractional divider
+	  and spread spectrum support.
+
 config COMMON_CLK_STM32MP135
 	def_bool COMMON_CLK && MACH_STM32MP13
 	help
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058..6b12861f7 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARCH_NPCM7XX)	    	+= clk-npcm7xx.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
 obj-$(CONFIG_COMMON_CLK_OXNAS)		+= clk-oxnas.o
 obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
+obj-$(CONFIG_COMMON_CLK_PHICLOCK)	+= clk-phiclock.o
 obj-$(CONFIG_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
diff --git a/drivers/clk/clk-phiclock.c b/drivers/clk/clk-phiclock.c
new file mode 100644
index 000000000..cff1547e6
--- /dev/null
+++ b/drivers/clk/clk-phiclock.c
@@ -0,0 +1,729 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common clock framework driver for the PhiClock 9FGV1006 clock generator.
+ *
+ * Copyright (c) 2022 Renesas Electronics Corporation
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/swab.h>
+
+/* VCO range is 2.3GHz to 2.6GHz */
+#define PHICLOCK_FVCO_MIN		2300000000UL
+#define PHICLOCK_FVCO_MAX		2600000000UL
+
+/* FOUT range is 10MHz to 325MHz */
+#define PHICLOCK_FOUT_MIN		10000000UL
+#define PHICLOCK_FOUT_MAX		325000000UL
+
+/* FPFD max frequency is 160MHz */
+#define PHICLOCK_FPFD_MAX		160000000UL
+
+#define PHICLOCK_NUM_CLKS		4
+#define PHICLOCK_REF0			0
+#define PHICLOCK_OUT			1
+#define PHICLOCK_OUT0			2
+#define PHICLOCK_OUT1			3
+
+#define PHICLOCK_FBFRAC_BITS		16
+#define PHICLOCK_FBFRAC_DIVISOR		BIT(PHICLOCK_FBFRAC_BITS)
+
+#define PHICLOCK_FBDIV_MIN		12
+#define PHICLOCK_FBDIV_MAX		255
+
+#define PHICLOCK_OUTDIV_MIN		8
+#define PHICLOCK_OUTDIV_MAX		4095
+
+#define PHICLOCK_REG_REF0		0x01
+#define PHICLOCK_REG_OUT0		0x05
+#define PHICLOCK_REG_OUT1		0x0b
+#define PHICLOCK_REG_SS_CNFG_PERIOD	0x10
+#define PHICLOCK_REG_FBINT		0x12
+#define PHICLOCK_REG_FBFRAC		0x13
+#define PHICLOCK_REG_SS_STEP		0x15
+#define PHICLOCK_REG_FBCNFG		0x18
+#define PHICLOCK_REG_CALIBRATION	0x1a
+#define PHICLOCK_REG_OUTDIV		0x21
+#define PHICLOCK_REG_DOUBLER		0x25
+
+#define PHICLOCK_REF_EN_MASK		0x40
+#define PHICLOCK_OUT_EN_MASK		0x80
+#define PHICLOCK_FBMODE_MASK		0x02
+#define PHICLOCK_OUTDIV_11_8_MASK	0xf0
+#define PHICLOCK_DOUBLER_MASK		0x20
+#define PHICLOCK_VCO_CAL_MASK		0x80
+#define PHICLOCK_SS_EN_MASK		0x80
+#define PHICLOCK_SS_PERIOD_11_8_MASK	0x0f
+
+enum phiclock_pll_mode {
+	PLL_MODE_FRAC,
+	PLL_MODE_INT,
+};
+
+enum phiclock_ss_direction {
+	SS_DOWN,
+	SS_CENTER,
+};
+
+enum phiclock_model {
+	PHICLOCK_9FGV1006,
+};
+
+struct phiclock_data;
+
+struct phiclock_clk_hw {
+	struct clk_hw hw;
+	struct clk_init_data init;
+	struct phiclock_data *phiclock;
+	unsigned int idx;
+	u8 enabled;
+};
+
+struct phiclock_data {
+	struct regmap *regmap;
+	struct i2c_client *i2c_client;
+	struct clk *xin_clkin_clk;
+	struct phiclock_clk_hw hw[PHICLOCK_NUM_CLKS];
+	enum phiclock_model model;
+	enum phiclock_ss_direction ss_direction;
+	u16 ss_amount;
+	u16 ss_modulation;
+	u32 fvco;
+	u32 fout;
+	u8 doubler;
+	u8 fbint;
+	u16 fbfrac;
+	u16 outdiv;
+	u8 ss_en;
+	u16 ss_period;
+	u16 ss_step;
+};
+
+static inline struct phiclock_clk_hw *to_phiclock_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct phiclock_clk_hw, hw);
+}
+
+static int phiclock_get_en_reg(unsigned int idx, unsigned int *reg, unsigned int *mask)
+{
+	switch (idx) {
+	case PHICLOCK_REF0:
+		*reg = PHICLOCK_REG_REF0;
+		*mask = PHICLOCK_REF_EN_MASK;
+		break;
+	case PHICLOCK_OUT0:
+		*reg = PHICLOCK_REG_OUT0;
+		*mask = PHICLOCK_OUT_EN_MASK;
+		break;
+	case PHICLOCK_OUT1:
+		*reg = PHICLOCK_REG_OUT1;
+		*mask = PHICLOCK_OUT_EN_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static u32 phiclock_calc_fvco(struct phiclock_data *phiclock)
+{
+	u64 fref, fvco;
+	u8 doubler;
+
+	doubler = phiclock->doubler ? 2 : 1;
+	fref = clk_get_rate(phiclock->xin_clkin_clk) * doubler;
+
+	fvco = 2 * ((fref * phiclock->fbint) +
+		div_u64(fref * phiclock->fbfrac, PHICLOCK_FBFRAC_DIVISOR));
+
+	return (u32)fvco;
+}
+
+static int phiclock_get_divs(struct phiclock_data *phiclock, u8 *doubler, u8 *fbint, u16 *fbfrac,
+			     u16 *outdiv)
+{
+	int ret;
+	unsigned int tmp;
+	u8 reg[3];
+
+	ret = regmap_bulk_read(phiclock->regmap, PHICLOCK_REG_FBINT, reg, 3);
+	if (ret)
+		return ret;
+
+	*fbint = reg[0];
+	*fbfrac = (reg[1] << 8) + reg[2];
+
+	ret = regmap_bulk_read(phiclock->regmap, PHICLOCK_REG_OUTDIV, reg, 2);
+	if (ret)
+		return ret;
+
+	*outdiv = ((reg[1] & PHICLOCK_OUTDIV_11_8_MASK) << 8) + reg[0];
+	if (*outdiv < PHICLOCK_OUTDIV_MIN)
+		*outdiv = PHICLOCK_OUTDIV_MIN;
+	if (*outdiv > PHICLOCK_OUTDIV_MAX)
+		*outdiv = PHICLOCK_OUTDIV_MAX;
+
+	ret = regmap_read(phiclock->regmap, PHICLOCK_REG_DOUBLER, &tmp);
+	if (ret)
+		return ret;
+
+	*doubler = (tmp & PHICLOCK_DOUBLER_MASK) >> 5;
+
+	pr_debug("doubler: %u, fbint: %u, fbfrac: %u, outdiv: %u\n",
+		 *doubler, *fbint, *fbfrac, *outdiv);
+
+	return ret;
+}
+
+static int phiclock_get_ss(struct phiclock_data *phiclock, u8 *ss_en, u16 *ss_period, u16 *ss_step)
+{
+	int ret;
+	u8 reg[2];
+
+	*ss_en = 0;
+	*ss_period = 0;
+	*ss_step = 0;
+
+	ret = regmap_bulk_read(phiclock->regmap, PHICLOCK_REG_SS_CNFG_PERIOD, reg, 2);
+	if (ret)
+		return ret;
+
+	*ss_en = !!(reg[0] & PHICLOCK_SS_EN_MASK);
+	*ss_period = ((reg[0] & PHICLOCK_SS_PERIOD_11_8_MASK) << 8) + reg[1];
+
+	ret = regmap_bulk_read(phiclock->regmap, PHICLOCK_REG_SS_STEP, reg, 2);
+	if (ret)
+		return ret;
+
+	*ss_step = (reg[0] << 8) + reg[1];
+
+	pr_debug("ss_en: %u, ss_period: %u, ss_step: %u\n", *ss_en, *ss_period, *ss_step);
+
+	return ret;
+}
+
+static int phiclock_get_defaults(struct phiclock_data *phiclock)
+{
+	int ret;
+	unsigned int ref0, out0, out1;
+
+	ret = phiclock_get_divs(phiclock, &phiclock->doubler, &phiclock->fbint, &phiclock->fbfrac,
+				&phiclock->outdiv);
+	if (ret)
+		return ret;
+
+	phiclock->fvco = phiclock_calc_fvco(phiclock);
+	phiclock->fout = phiclock->fvco / phiclock->outdiv;
+
+	ret = phiclock_get_ss(phiclock, &phiclock->ss_en, &phiclock->ss_period, &phiclock->ss_step);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(phiclock->regmap, PHICLOCK_REG_REF0, &ref0);
+	ret = regmap_read(phiclock->regmap, PHICLOCK_REG_OUT0, &out0);
+	ret = regmap_read(phiclock->regmap, PHICLOCK_REG_OUT1, &out1);
+	if (ret)
+		return ret;
+
+	ref0 = (ref0 & PHICLOCK_REF_EN_MASK) >> __ffs(PHICLOCK_REF_EN_MASK);
+	out0 = (out0 & PHICLOCK_OUT_EN_MASK) >> __ffs(PHICLOCK_OUT_EN_MASK);
+	out1 = (out1 & PHICLOCK_OUT_EN_MASK) >> __ffs(PHICLOCK_OUT_EN_MASK);
+	phiclock->hw[PHICLOCK_REF0].enabled = ref0;
+	phiclock->hw[PHICLOCK_OUT0].enabled = out0;
+	phiclock->hw[PHICLOCK_OUT1].enabled = out1;
+
+	pr_debug("doubler: %u, fbint: %u, fbfrac: %u, outdiv: %u, fvco: %u, fout: %u\n",
+		 phiclock->doubler, phiclock->fbint, phiclock->fbfrac, phiclock->outdiv,
+		 phiclock->fvco, phiclock->fout);
+
+	pr_debug("ref0_en: %u, out0_en: %u, out1_en: %u\n", ref0, out0, out1);
+
+	pr_debug("ss_en: %u, ss_period: %u, ss_step: %u\n",
+		 phiclock->ss_en, phiclock->ss_period, phiclock->ss_step);
+
+	return ret;
+}
+
+static int phiclock_calc_divs(const unsigned long frequency, const struct phiclock_data *phiclock,
+			      u8 *doubler, u8 *fbint, u16 *fbfrac, u16 *outdiv, u32 *fout,
+			      u32 *fvco, u16 *ss_period, u16 *ss_step)
+{
+	u16 outdivstart;
+	u32 fpfd;
+	unsigned long fin;
+	bool found = false, allow_frac = false;
+	bool ss_en = phiclock->ss_en && phiclock->ss_amount && phiclock->ss_modulation;
+
+	if (frequency == 0)
+		return -EINVAL;
+
+	if (phiclock->fbint == 0)
+		return -EINVAL;
+
+	fin = clk_get_rate(phiclock->xin_clkin_clk);
+	*doubler = 2 * fin <= PHICLOCK_FPFD_MAX ? 1 : 0;
+	*fbint = PHICLOCK_FBDIV_MIN;
+	*fbfrac = 0;
+	*outdiv = PHICLOCK_OUTDIV_MIN;
+	*fout = PHICLOCK_FOUT_MIN;
+	*fvco = *outdiv * frequency;
+
+	fpfd = fin * (*doubler + 1);
+	outdivstart = 1 + (PHICLOCK_FVCO_MIN / frequency);
+
+	if (ss_en)
+		allow_frac = true;
+
+retry:
+	for (*outdiv = outdivstart; *outdiv <= PHICLOCK_OUTDIV_MAX; ++(*outdiv)) {
+		*fvco = *outdiv * frequency;
+		if (*fvco > PHICLOCK_FVCO_MAX) {
+			allow_frac = true;
+			goto retry;
+		}
+
+		*fbint = (*fvco >> 1) / fpfd;
+		*fbfrac = (*fvco >> 1) % fpfd;
+
+		if (*fbint < PHICLOCK_FBDIV_MIN)
+			*fbint = PHICLOCK_FBDIV_MIN;
+		if (*fbint > PHICLOCK_FBDIV_MAX)
+			*fbint = PHICLOCK_FBDIV_MAX;
+
+		if (*fbfrac == 0) {
+			found = true;
+			break;
+		}
+
+		if (allow_frac) {
+			*fbfrac = 1 + div_u64(((u64)*fvco >> 1) % fpfd << PHICLOCK_FBFRAC_BITS,
+					      fpfd);
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	if (*fvco < PHICLOCK_FVCO_MIN || *fvco > PHICLOCK_FVCO_MAX)
+		return -EINVAL;
+
+	if (ss_en) {
+		unsigned int ss_amount = phiclock->ss_amount;
+
+		if (phiclock->ss_direction == SS_CENTER) {
+			u64 num, den;
+			u32 rem;
+
+			num = (u64)*fbint * PHICLOCK_FBFRAC_DIVISOR + *fbfrac;
+			num *= 10000 + phiclock->ss_amount;
+			den = 10000 * PHICLOCK_FBFRAC_DIVISOR;
+			*fbint = div_u64_rem(num, den, &rem);
+			*fbfrac = rem / 10000;
+
+			/* center spread requires peak-peak */
+			ss_amount *= 2;
+		}
+
+		*ss_period = fpfd / (4 * phiclock->ss_modulation);
+		*ss_step = (u16)div_u64((ss_amount * (u64)*fbint) << 24, 20000 * *ss_period);
+	}
+
+	*fvco = 2 * (((u64)fpfd * *fbint) + div_u64((u64)fpfd * *fbfrac, PHICLOCK_FBFRAC_DIVISOR));
+	*fout = *fvco / *outdiv;
+
+	if (*fout < PHICLOCK_FOUT_MIN || *fout > PHICLOCK_FOUT_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int phiclock_update_frequency(struct phiclock_data *phiclock)
+{
+	int ret;
+	enum phiclock_pll_mode pll_mode;
+	u8 reg[3];
+
+	pll_mode = phiclock->fbfrac == 0 ? PLL_MODE_INT : PLL_MODE_FRAC;
+
+	if (phiclock->ss_period == 0 && phiclock->ss_step == 0)
+		phiclock->ss_en = 0;
+
+	reg[0] = phiclock->fbint;
+	reg[1] = (phiclock->fbfrac >> 8) & 0xff;
+	reg[2] = phiclock->fbfrac & 0xff;
+	ret = regmap_bulk_write(phiclock->regmap, PHICLOCK_REG_FBINT, reg, 3);
+
+	reg[0] = phiclock->outdiv & 0xff;
+	reg[1] = (phiclock->outdiv & PHICLOCK_OUTDIV_11_8_MASK) >> 4;
+	ret = regmap_bulk_write(phiclock->regmap, PHICLOCK_REG_OUTDIV, reg, 2);
+
+	ret = regmap_write_bits(phiclock->regmap, PHICLOCK_REG_FBCNFG, PHICLOCK_FBMODE_MASK,
+				pll_mode << __ffs(PHICLOCK_FBMODE_MASK));
+
+	ret = regmap_write_bits(phiclock->regmap, PHICLOCK_REG_DOUBLER, PHICLOCK_DOUBLER_MASK,
+				phiclock->doubler << __ffs(PHICLOCK_DOUBLER_MASK));
+
+	ret = regmap_write_bits(phiclock->regmap, PHICLOCK_REG_SS_CNFG_PERIOD, PHICLOCK_SS_EN_MASK,
+				phiclock->ss_en << __ffs(PHICLOCK_SS_EN_MASK));
+
+	reg[0] = phiclock->ss_en << __ffs(PHICLOCK_SS_EN_MASK);
+	reg[0] += ((phiclock->ss_period >> 8) & PHICLOCK_SS_PERIOD_11_8_MASK);
+	reg[1] = phiclock->ss_period & 0xff;
+	ret = regmap_bulk_write(phiclock->regmap, PHICLOCK_REG_SS_CNFG_PERIOD, reg, 2);
+
+	reg[0] = (phiclock->ss_step >> 8) & 0xff;
+	reg[1] = phiclock->ss_step & 0xff;
+	ret = regmap_bulk_write(phiclock->regmap, PHICLOCK_REG_SS_STEP, reg, 2);
+
+	/* VCO calibration to apply the changes, cal starts on the 0->1 transition */
+	ret = regmap_write_bits(phiclock->regmap, PHICLOCK_REG_CALIBRATION, PHICLOCK_VCO_CAL_MASK,
+				0 << __ffs(PHICLOCK_VCO_CAL_MASK));
+	ret = regmap_write_bits(phiclock->regmap, PHICLOCK_REG_CALIBRATION, PHICLOCK_VCO_CAL_MASK,
+				1 << __ffs(PHICLOCK_VCO_CAL_MASK));
+
+	return ret;
+}
+
+static int phiclock_set_frequency(struct phiclock_data *phiclock, unsigned long frequency)
+{
+	int ret;
+
+	ret = phiclock_calc_divs(frequency, phiclock,
+				 &phiclock->doubler, &phiclock->fbint, &phiclock->fbfrac,
+				 &phiclock->outdiv, &phiclock->fout, &phiclock->fvco,
+				 &phiclock->ss_period, &phiclock->ss_step);
+	if (ret)
+		return ret;
+
+	pr_debug("doubler: %u, fbint: %u, fbfrac: %u, outdiv: %u, fvco: %u, fout: %u\n",
+		 phiclock->doubler, phiclock->fbint, phiclock->fbfrac, phiclock->outdiv,
+		 phiclock->fvco, phiclock->fout);
+
+	if (phiclock->ss_en)
+		pr_debug("ss_period: %u, ss_step: %u\n",
+			 phiclock->ss_period, phiclock->ss_step);
+
+	ret = phiclock_update_frequency(phiclock);
+
+	return ret;
+}
+
+static unsigned long phiclock_clk_recalc_rate(struct clk_hw *hw, unsigned long rate)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+	int ret;
+	u8 doubler, fbint;
+	u16 fbfrac, outdiv;
+	u32 fvco, fout;
+
+	ret = phiclock_get_divs(phiclock, &doubler, &fbint, &fbfrac, &outdiv);
+	if (ret) {
+		dev_err(&phiclock->i2c_client->dev, "recalc rate error\n");
+		return 0;
+	}
+
+	fvco = phiclock_calc_fvco(phiclock);
+	fout = fvco / outdiv;
+
+	return fout;
+}
+
+static long phiclock_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *parent_rate)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+	int ret;
+	u8 doubler, fbint, ss_en;
+	u16 fbfrac, outdiv, ss_period, ss_step;
+	u32 fout, fvco;
+
+	if (!rate)
+		return 0;
+
+	/* Temporarily disable spread spectrum for rounding. */
+	ss_en = phiclock->ss_en;
+	phiclock->ss_en = 0;
+	ret = phiclock_calc_divs(rate, phiclock, &doubler, &fbint, &fbfrac, &outdiv, &fout, &fvco,
+				 &ss_period, &ss_step);
+	phiclock->ss_en = ss_en;
+	if (ret) {
+		dev_err(&phiclock->i2c_client->dev, "round rate error\n");
+		return 0;
+	}
+
+	return fout;
+}
+
+static int phiclock_clk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+
+	if (rate < PHICLOCK_FOUT_MIN || rate > PHICLOCK_FOUT_MAX) {
+		dev_err(&phiclock->i2c_client->dev, "requested frequency %lu Hz is out of range\n",
+			rate);
+		return -EINVAL;
+	}
+
+	return phiclock_set_frequency(phiclock, rate);
+}
+
+static int phiclock_clk_prepare(struct clk_hw *hw)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+	int err;
+	unsigned int reg, mask;
+
+	pr_debug("prepare %s", clk_hw_get_name(hw));
+
+	err = phiclock_get_en_reg(clk_hw->idx, &reg, &mask);
+	if (err)
+		return err;
+
+	clk_hw->enabled = 1;
+	err = regmap_update_bits(phiclock->regmap, reg, mask, mask);
+	return err;
+}
+
+static void phiclock_clk_unprepare(struct clk_hw *hw)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+	int err;
+	unsigned int reg, mask;
+
+	pr_debug("unprepare %s", clk_hw_get_name(hw));
+
+	err = phiclock_get_en_reg(clk_hw->idx, &reg, &mask);
+	if (!err) {
+		clk_hw->enabled = 0;
+		regmap_update_bits(phiclock->regmap, reg, mask, 0);
+	}
+}
+
+static int phiclock_clk_is_prepared(struct clk_hw *hw)
+{
+	struct phiclock_clk_hw *clk_hw = to_phiclock_clk(hw);
+	struct phiclock_data *phiclock = clk_hw->phiclock;
+	int err;
+	unsigned int reg, mask, val = 0;
+
+	pr_debug("is_prepared %s", clk_hw_get_name(hw));
+
+	err = phiclock_get_en_reg(clk_hw->idx, &reg, &mask);
+	if (err)
+		return err;
+
+	err = regmap_read(phiclock->regmap, reg, &val);
+	if (err)
+		return err;
+
+	return !!(val & mask);
+}
+
+static const struct i2c_device_id phiclock_i2c_id[] = {
+	{ "9fgv1006", PHICLOCK_9FGV1006 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, phiclock_i2c_id);
+
+static const struct regmap_config phiclock_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x25,
+	.cache_type = REGCACHE_RBTREE
+};
+
+struct phiclock_clk {
+	const char *name;
+	int parent_index;
+	const struct clk_ops ops;
+};
+
+static const struct phiclock_clk phiclock_clks[PHICLOCK_NUM_CLKS] = {
+	[PHICLOCK_REF0] = {
+		.name = "phiclock_ref0",
+		.parent_index = -1,
+		.ops = {
+			.prepare	= phiclock_clk_prepare,
+			.unprepare	= phiclock_clk_unprepare,
+			.is_prepared	= phiclock_clk_is_prepared,
+		},
+	},
+	[PHICLOCK_OUT] = {
+		.name = "phiclock_out",
+		.parent_index = -1,
+		.ops = {
+			.recalc_rate	= phiclock_clk_recalc_rate,
+			.round_rate	= phiclock_clk_round_rate,
+			.set_rate	= phiclock_clk_set_rate,
+		},
+	},
+	[PHICLOCK_OUT0] = {
+		.name = "phiclock_out0",
+		.parent_index = PHICLOCK_OUT,
+		.ops = {
+			.prepare	= phiclock_clk_prepare,
+			.unprepare	= phiclock_clk_unprepare,
+			.is_prepared	= phiclock_clk_is_prepared,
+		},
+	},
+	[PHICLOCK_OUT1] = {
+		.name = "phiclock_out1",
+		.parent_index = PHICLOCK_OUT,
+		.ops = {
+			.prepare	= phiclock_clk_prepare,
+			.unprepare	= phiclock_clk_unprepare,
+			.is_prepared	= phiclock_clk_is_prepared,
+		},
+	},
+};
+
+static struct clk_hw *phiclock_of_clk_get(struct of_phandle_args *clkspec, void *data)
+{
+	struct phiclock_data *driver_data = data;
+	unsigned int idx = clkspec->args[0];
+
+	return &driver_data->hw[idx].hw;
+}
+
+static int phiclock_probe(struct i2c_client *client)
+{
+	struct phiclock_data *phiclock;
+	struct device *dev = &client->dev;
+	const char *xin_clkin_name, *spread_spectrum_type_name;
+	unsigned int i;
+	u32 ss_amount = 0, ss_modulation = 0;
+	enum phiclock_ss_direction ss_direction = SS_DOWN;
+	int ret;
+
+	phiclock = devm_kzalloc(dev, sizeof(*phiclock), GFP_KERNEL);
+	if (!phiclock)
+		return -ENOMEM;
+
+	phiclock->xin_clkin_clk = devm_clk_get(dev, "xin-clkin");
+	if (IS_ERR(phiclock->xin_clkin_clk)) {
+		return dev_err_probe(dev, PTR_ERR(phiclock->xin_clkin_clk),
+				     "xin-clkin not specified\n");
+	}
+
+	xin_clkin_name = __clk_get_name(phiclock->xin_clkin_clk);
+
+	phiclock->regmap = devm_regmap_init_i2c(client, &phiclock_regmap_config);
+	if (IS_ERR(phiclock->regmap)) {
+		return dev_err_probe(dev, PTR_ERR(phiclock->regmap),
+				     "failed to initialize register map\n");
+	}
+
+	i2c_set_clientdata(client, phiclock);
+	phiclock->i2c_client = client;
+	phiclock->ss_amount = ss_amount;
+	phiclock->ss_modulation = ss_modulation;
+	phiclock->ss_direction = ss_direction;
+
+	ret = phiclock_get_defaults(phiclock);
+	if (ret) {
+		dev_err(dev, "error getting defaults\n");
+		return ret;
+	}
+
+	for (i = 0; i < PHICLOCK_NUM_CLKS; i++) {
+		int parent_index = phiclock_clks[i].parent_index;
+		const char *name;
+
+		if (of_property_read_string_index(dev->of_node,
+						  "clock-output-names", i, &name) == 0) {
+			phiclock->hw[i].init.name = name;
+		} else {
+			phiclock->hw[i].init.name = phiclock_clks[i].name;
+		}
+
+		phiclock->hw[i].idx = i;
+		phiclock->hw[i].init.ops = &phiclock_clks[i].ops;
+		phiclock->hw[i].init.num_parents = 1;
+		phiclock->hw[i].init.flags = 0;
+
+		if (parent_index > 0) {
+			phiclock->hw[i].init.parent_names = &phiclock->hw[parent_index].init.name;
+			phiclock->hw[i].init.flags |= CLK_SET_RATE_PARENT;
+		} else {
+			phiclock->hw[i].init.parent_names = &xin_clkin_name;
+		}
+
+		phiclock->hw[i].hw.init = &phiclock->hw[i].init;
+		phiclock->hw[i].phiclock = phiclock;
+
+		ret = devm_clk_hw_register(dev, &phiclock->hw[i].hw);
+		if (ret < 0) {
+			return dev_err_probe(dev, ret, "failed to register %s clock\n",
+					     phiclock->hw[i].init.name);
+		}
+	}
+
+	if (of_property_read_u32(dev->of_node, "renesas,ss-amount-percent", &ss_amount) == 0)
+		phiclock->ss_amount = ss_amount;
+
+	if (of_property_read_u32(dev->of_node, "renesas,ss-modulation-hz", &ss_modulation) == 0)
+		phiclock->ss_modulation = ss_modulation;
+
+	if (of_property_read_string(dev->of_node, "renesas,ss-direction",
+				    &spread_spectrum_type_name) == 0) {
+		if (strcmp(spread_spectrum_type_name, "center") == 0)
+			phiclock->ss_direction = SS_CENTER;
+		else
+			phiclock->ss_direction = SS_DOWN;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, phiclock_of_clk_get, phiclock);
+	if (ret) {
+		dev_err(dev, "unable to add clk provider\n");
+		return ret;
+	}
+
+	dev_info(dev, "registered, current frequency %u Hz\n", phiclock->fout);
+
+	return ret;
+}
+
+static void phiclock_remove(struct i2c_client *client)
+{
+	of_clk_del_provider(client->dev.of_node);
+}
+
+static const struct of_device_id phiclock_of_match[] = {
+	{ .compatible = "renesas,9fgv1006" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, phiclock_of_match);
+
+static struct i2c_driver phiclock_i2c_driver = {
+	.driver = {
+		.name = "phiclock",
+		.of_match_table = phiclock_of_match,
+	},
+	.probe_new = phiclock_probe,
+	.remove = phiclock_remove,
+	.id_table = phiclock_i2c_id,
+};
+module_i2c_driver(phiclock_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alex Helms <alexander.helms.jy@renesas.com");
+MODULE_DESCRIPTION("Renesas PhiClock Clock Generator Driver");
-- 
2.25.1

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Krzysztof Kozlowski]

On 15/11/2022 20:26, Alex Helms wrote:
> Add dt bindings for the Renesas PhiClock clock generator.
> 

Subject: drop second, redundant "bindings"

> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> ---
>  .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> new file mode 100644
> index 000000000..2b36534d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml

Filename based on compatible.

> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,phiclock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas PhiClock Clock Generator Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +maintainers:
> +  - Alex Helms <alexander.helms.jy@renesas.com>
> +
> +description: |
> +  The Renesas PhiClock is a programmable I2C clock generator that provides
> +  1 reference output and 2 clock outputs.
> +
> +  The driver supports spread spectrum but only if all configurations use the

Driver as in Linux driver? Drop entire paragraph. Bindings are about
hardware, not driver.

> +  same spread spectrum parameters. If your configuration uses spread spectrum,
> +  you must include renesas,ss-amount-percent, renesas,ss-modulation-hz, and
> +  renesas,ss-direction in the device tree.
> +
> +properties:

compatible goes always first. Start your schema from example-schema.yaml.

> +  '#clock-cells':
> +    const: 1
> +
> +  clock-names:
> +    items:
> +      - const: xin-clkin

Just "xin" or entirely drop.

> +
> +  clocks:
> +    const: 1
> +
> +  compatible:
> +    enum:
> +      - renesas,9fgv1006
> +
> +  reg:
> +    maxItems: 1
> +
> +  renesas,ss-amount-percent:
> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.

What? If this is percent then it cannot be hundreds of percent. Percent
is percent. Use appropriate units.
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    minimum: 0
> +    maximum: 500
> +
> +  renesas,ss-modulation-hz:
> +    description: Spread spectrum modulation rate in Hz
> +    minimum: 30000
> +    maximum: 63000
> +
> +  renesas,ss-direction:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Spread spectrum direction
> +    enum: [ down, center ]
> +
> +required:
> +  - clock-names
> +  - '#clock-cells'
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ref25: ref25m {
> +      compatible = "fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency = <25000000>;
> +    };

Drop, it's obvious, isn't it?

> +

Best regards,
Krzysztof

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, Nov 16, 2022 at 9:20 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 15/11/2022 20:26, Alex Helms wrote:
> > Add dt bindings for the Renesas PhiClock clock generator.
> >
>
> Subject: drop second, redundant "bindings"
>
> > Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> > ---
> >  .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
> >  MAINTAINERS                                   |  5 ++
> >  2 files changed, 86 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> > new file mode 100644
> > index 000000000..2b36534d3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>
> Filename based on compatible.

Looks like there are more of them, so I guess the family name is fine?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Krzysztof Kozlowski]

On 16/11/2022 09:34, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Wed, Nov 16, 2022 at 9:20 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 15/11/2022 20:26, Alex Helms wrote:
>>> Add dt bindings for the Renesas PhiClock clock generator.
>>>
>>
>> Subject: drop second, redundant "bindings"
>>
>>> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
>>> ---
>>>  .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
>>>  MAINTAINERS                                   |  5 ++
>>>  2 files changed, 86 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>>> new file mode 100644
>>> index 000000000..2b36534d3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>>
>> Filename based on compatible.
> 
> Looks like there are more of them, so I guess the family name is fine?
>

If you are sure, then yes. Can we add them now? If not, it's also fine.

Best regards,
Krzysztof

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Alex Helms]

On 11/16/2022 1:20 AM, Krzysztof Kozlowski wrote:
> On 15/11/2022 20:26, Alex Helms wrote:
>> Add dt bindings for the Renesas PhiClock clock generator.
>>
> 
> Subject: drop second, redundant "bindings"
> 
>> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
>> ---
>>  .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
>>  MAINTAINERS                                   |  5 ++
>>  2 files changed, 86 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>> new file mode 100644
>> index 000000000..2b36534d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> 
> Filename based on compatible.
> 

As Geert mentioned in the other thread, this is a family of products but
the others cannot be added now.

>> @@ -0,0 +1,81 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Frenesas%2Cphiclock.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=J6kNqua%2FJf0c8HczRM8gU8%2Fm%2BhX6gSF2fqnf2n3wSbI%3D&amp;reserved=0
>> +$schema: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ofZbC2sBnTpJR3KKzPVqhsFy28r4JjbJpaVGSuHKx38%3D&amp;reserved=0
>> +
>> +title: Renesas PhiClock Clock Generator Device Tree Bindings
> 
> Drop "Device Tree Bindings"
> 
>> +
>> +maintainers:
>> +  - Alex Helms <alexander.helms.jy@renesas.com>
>> +
>> +description: |
>> +  The Renesas PhiClock is a programmable I2C clock generator that provides
>> +  1 reference output and 2 clock outputs.
>> +
>> +  The driver supports spread spectrum but only if all configurations use the
> 
> Driver as in Linux driver? Drop entire paragraph. Bindings are about
> hardware, not driver.
> 
>> +  same spread spectrum parameters. If your configuration uses spread spectrum,
>> +  you must include renesas,ss-amount-percent, renesas,ss-modulation-hz, and
>> +  renesas,ss-direction in the device tree.
>> +
>> +properties:
> 
> compatible goes always first. Start your schema from example-schema.yaml.
> 
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xin-clkin
> 
> Just "xin" or entirely drop.

The pin name on the datasheet is "xin-clkin" and as the name implies it
can be a crystal or clock input. If the name were different it could be
confusing.

> 
>> +
>> +  clocks:
>> +    const: 1
>> +
>> +  compatible:
>> +    enum:
>> +      - renesas,9fgv1006
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  renesas,ss-amount-percent:
>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
> 
> What? If this is percent then it cannot be hundreds of percent. Percent
> is percent. Use appropriate units.
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&amp;reserved=0
> 

Values like 0.5% or 2.5% must be representable which is why this
property is an integer of hundredths of percent. How else would you
represent a non-integer percent?

>> +    minimum: 0
>> +    maximum: 500
>> +
>> +  renesas,ss-modulation-hz:
>> +    description: Spread spectrum modulation rate in Hz
>> +    minimum: 30000
>> +    maximum: 63000
>> +
>> +  renesas,ss-direction:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Spread spectrum direction
>> +    enum: [ down, center ]
>> +
>> +required:
>> +  - clock-names
>> +  - '#clock-cells'
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    ref25: ref25m {
>> +      compatible = "fixed-clock";
>> +      #clock-cells = <0>;
>> +      clock-frequency = <25000000>;
>> +    };
> 
> Drop, it's obvious, isn't it?
> 

I disagree, this may be obvious to someone familiar with how clocks in
the device tree works but not long ago it was entirely new to me and
examples like these in the dt schemas were very helpful in getting the
device up and running. There are several other bindings that define
external crystals and reference clocks in this way.

>> +
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Krzysztof Kozlowski]

On 16/11/2022 21:11, Alex Helms wrote:
>>> +  clocks:
>>> +    const: 1
>>> +
>>> +  compatible:
>>> +    enum:
>>> +      - renesas,9fgv1006
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  renesas,ss-amount-percent:
>>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
>>
>> What? If this is percent then it cannot be hundreds of percent. Percent
>> is percent. Use appropriate units.
>> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&reserved=0
>>
> 
> Values like 0.5% or 2.5% must be representable which is why this
> property is an integer of hundredths of percent. How else would you
> represent a non-integer percent?

With an appropriate unit.

> 
>>> +    minimum: 0
>>> +    maximum: 500
>>> +
>>> +  renesas,ss-modulation-hz:
>>> +    description: Spread spectrum modulation rate in Hz
>>> +    minimum: 30000
>>> +    maximum: 63000
>>> +
>>> +  renesas,ss-direction:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description: Spread spectrum direction
>>> +    enum: [ down, center ]
>>> +
>>> +required:
>>> +  - clock-names
>>> +  - '#clock-cells'
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    ref25: ref25m {
>>> +      compatible = "fixed-clock";
>>> +      #clock-cells = <0>;
>>> +      clock-frequency = <25000000>;
>>> +    };
>>
>> Drop, it's obvious, isn't it?
>>
> 
> I disagree, this may be obvious to someone familiar with how clocks in
> the device tree works but not long ago it was entirely new to me and
> examples like these in the dt schemas were very helpful in getting the
> device up and running. There are several other bindings that define
> external crystals and reference clocks in this way.

It is obvious because it is the same for every device being a consumer
of external clock. There is no point to duplicate non-device related
examples in every device binding.

Best regards,
Krzysztof

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Geert Uytterhoeven]

On Thu, Nov 17, 2022 at 8:39 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 16/11/2022 21:11, Alex Helms wrote:
> >>> +  clocks:
> >>> +    const: 1
> >>> +
> >>> +  compatible:
> >>> +    enum:
> >>> +      - renesas,9fgv1006
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  renesas,ss-amount-percent:
> >>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
> >>
> >> What? If this is percent then it cannot be hundreds of percent. Percent
> >> is percent. Use appropriate units.
> >> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&amp;reserved=0
> >>
> >
> > Values like 0.5% or 2.5% must be representable which is why this
> > property is an integer of hundredths of percent. How else would you
> > represent a non-integer percent?
>
> With an appropriate unit.

Krzysztof really means the property should be named e.g.
"renesas,ss-amount-centipercent" instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Krzysztof Kozlowski]

On 17/11/2022 09:11, Geert Uytterhoeven wrote:
>>>>> +
>>>>> +  renesas,ss-amount-percent:
>>>>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
>>>>
>>>> What? If this is percent then it cannot be hundreds of percent. Percent
>>>> is percent. Use appropriate units.
>>>> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&reserved=0
>>>>
>>>
>>> Values like 0.5% or 2.5% must be representable which is why this
>>> property is an integer of hundredths of percent. How else would you
>>> represent a non-integer percent?
>>
>> With an appropriate unit.
> 
> Krzysztof really means the property should be named e.g.
> "renesas,ss-amount-centipercent" instead.
> 

I provided an URL to document with units, so I was hoping Alex will take
a bit of effort to open it. Hundredths of percent sounds like basis
point... If it is not basis point, maybe it could be made such?

Best regards,
Krzysztof

Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Geert Uytterhoeven]

Hi Krzysztof,

On Thu, Nov 17, 2022 at 11:41 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 17/11/2022 09:11, Geert Uytterhoeven wrote:
> >>>>> +  renesas,ss-amount-percent:
> >>>>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
> >>>>
> >>>> What? If this is percent then it cannot be hundreds of percent. Percent
> >>>> is percent. Use appropriate units.
> >>>> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&amp;reserved=0
> >>>>
> >>>
> >>> Values like 0.5% or 2.5% must be representable which is why this
> >>> property is an integer of hundredths of percent. How else would you
> >>> represent a non-integer percent?
> >>
> >> With an appropriate unit.
> >
> > Krzysztof really means the property should be named e.g.
> > "renesas,ss-amount-centipercent" instead.
>
> I provided an URL to document with units, so I was hoping Alex will take
> a bit of effort to open it. Hundredths of percent sounds like basis
> point... If it is not basis point, maybe it could be made such?

Thank you, I didn't know about basis points. TIL ;-)

(and I didn't dare to click on the safelink-converted quoted link)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds