[PATCH v3 0/2] Add support for Renesas ProXO XP oscillator

[PATCH v3 0/2] Add support for Renesas ProXO XP oscillator

[Alex Helms]

Device tree bindings and a common clock framework device drivers
for the Renesas ProXO XP oscillator.

V3:
 - Add '-hz' suffix to dt property
V2:
 - Fix minor issues in dt bindings
 - Change renesas,xtal to renesas,crystal-frequency

Alex Helms (2):
  dt-bindings: clock: Add bindings for Renesas ProXO
  clk: Add support for Renesas ProXO oscillator

 .../bindings/clock/renesas,proxo.yaml         |  51 +++
 MAINTAINERS                                   |   6 +
 drivers/clk/Kconfig                           |   7 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-proxo.c                       | 410 ++++++++++++++++++
 5 files changed, 475 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,proxo.yaml
 create mode 100644 drivers/clk/clk-proxo.c


base-commit: f64b5666e11dce481737208027d4af300c63842d
-- 
2.30.2

[PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO

[Alex Helms]

Add dt bindings for the Renesas ProXO oscillator.

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

diff --git a/Documentation/devicetree/bindings/clock/renesas,proxo.yaml b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
new file mode 100644
index 000000000..ff960196d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,proxo.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas ProXO Oscillator Device Tree Bindings
+
+maintainers:
+  - Alex Helms <alexander.helms.jy@renesas.com>
+
+description:
+  Renesas ProXO is a family of programmable ultra-low phase noise
+  quartz-based oscillators.
+
+properties:
+  '#clock-cells':
+    const: 0
+
+  compatible:
+    enum:
+      - renesas,proxo-xp
+
+  reg:
+    maxItems: 1
+
+  clock-output-names:
+    maxItems: 1
+
+  renesas,crystal-frequency-hz:
+    description: Internal crystal frequency, default is 50000000 (50MHz)
+
+required:
+  - '#clock-cells'
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      proxo: clock-controller@55 {
+        compatible = "renesas,proxo-xp";
+        reg = <0x55>;
+        #clock-cells = <0>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 350102355..d52a8a5d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16080,6 +16080,11 @@ S:	Supported
 F:	Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
 F:	drivers/iio/adc/rzg2l_adc.c
 
+RENESAS PROXO CLOCK DRIVER
+M:	Alex Helms <alexander.helms.jy@renesas.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/clock/renesas,proxo.yaml
+
 RESET CONTROLLER FRAMEWORK
 M:	Philipp Zabel <p.zabel@pengutronix.de>
 S:	Maintained
-- 
2.30.2

[PATCH v3 2/2] clk: Add support for Renesas ProXO oscillator

[Alex Helms]

ProXO is a programmable ultra-low phase noise quartz-based
oscillator with a single output. This driver supports changing
the frequency of the ProXP XP variant.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index d52a8a5d2..c7f5c0655 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16084,6 +16084,7 @@ RENESAS PROXO CLOCK DRIVER
 M:	Alex Helms <alexander.helms.jy@renesas.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/renesas,proxo.yaml
+F:	drivers/clk/clk-proxo.c
 
 RESET CONTROLLER FRAMEWORK
 M:	Philipp Zabel <p.zabel@pengutronix.de>
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 8f905df60..31f684d69 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -396,6 +396,13 @@ config COMMON_CLK_K210
 	help
 	  Support for the Canaan Kendryte K210 RISC-V SoC clocks.
 
+config COMMON_CLK_PROXO
+	bool "Clock driver for Renesas ProXO"
+	depends on I2C && OF
+	select REGMAP_I2C
+	help
+	  Support for the Renesas ProXO oscillator clock.
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/analogbits/Kconfig"
 source "drivers/clk/baikal-t1/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 99941b4a3..be6e28cc4 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -52,6 +52,7 @@ 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_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
+obj-$(CONFIG_COMMON_CLK_PROXO)		+= clk-proxo.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
diff --git a/drivers/clk/clk-proxo.c b/drivers/clk/clk-proxo.c
new file mode 100644
index 000000000..7d4268687
--- /dev/null
+++ b/drivers/clk/clk-proxo.c
@@ -0,0 +1,410 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common clock framework driver for the ProXO family of quartz-based oscillators.
+ *
+ * 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/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/swab.h>
+
+/* Most ProXO products have a 50MHz xtal, can be overridden in device tree */
+#define PROXO_DEFAULT_XTAL	50000000
+
+/* VCO range is 6.86 GHz to 8.65 GHz */
+#define PROXO_FVCO_MIN		6860000000ULL
+#define PROXO_FVCO_MAX		8650000000ULL
+
+/* Output range is 15MHz to 2.1GHz */
+#define PROXO_FOUT_MIN		15000000UL
+#define PROXO_FOUT_MAX		2100000000UL
+
+#define PROXO_FRAC_BITS		24
+#define PROXO_FRAC_DIVISOR	BIT(PROXO_FRAC_BITS)
+
+/* Disable the doubler if the crystal is > 80MHz */
+#define PROXO_FDBL_MAX		80000000U
+
+#define PROXO_OUTDIV_MIN	4
+#define PROXO_OUTDIV_MAX	511
+#define PROXO_FB_MIN		41
+
+#define PROXO_REG_FREQ0		0x10
+#define PROXO_REG_XO		0x51
+#define PROXO_REG_TRIG		0x62
+
+#define OUTDIV_8_MASK		0x80
+#define FBDIV_INT_8_7_MASK	0x30
+#define FBDIV_INT_6_0_MASK	0x7f
+#define DOUBLE_DIS_MASK		0x80
+#define CP_MASK			0x0e
+#define PLL_MODE_MASK		0x01
+
+enum proxo_model {
+	PROXO_XP,
+};
+
+enum proxo_pll_mode {
+	PLL_MODE_FRAC,
+	PLL_MODE_INT,
+};
+
+struct clk_proxo {
+	struct clk_hw hw;
+	struct regmap *regmap;
+	struct i2c_client *i2c_client;
+	enum proxo_model model;
+	u32 fxtal;
+	u64 fvco;
+	u32 fout;
+	u8 double_dis;
+	u16 fb_int;
+	u32 fb_frac;
+	u16 out_div;
+};
+
+#define to_clk_proxo(_hw)	container_of(_hw, struct clk_proxo, hw)
+
+static u8 proxo_get_cp_value(u64 fvco)
+{
+	if (fvco < 7000000000ULL)
+		return 5;
+	else if (fvco >= 7000000000ULL && fvco < 7400000000ULL)
+		return 4;
+	else if (fvco >= 7400000000ULL && fvco < 7800000000ULL)
+		return 3;
+	else
+		return 2;
+}
+
+static u64 proxo_calc_fvco(u32 fxtal, u8 double_dis, u16 fb_int, u32 fb_frac)
+{
+	u64 fref, fvco;
+	u8 doubler;
+
+	doubler = double_dis ? 1 : 2;
+	fref = (u64)fxtal * doubler;
+	fvco = (fref * fb_int) + div_u64(fref * fb_frac, PROXO_FRAC_DIVISOR);
+
+	return fvco;
+}
+
+static int proxo_get_divs(struct clk_proxo *proxo, u16 *out_div, u16 *fb_int, u32 *fb_frac,
+			  u8 *double_dis)
+{
+	int ret;
+	u8 reg[6];
+	unsigned int xo;
+
+	ret = regmap_bulk_read(proxo->regmap, PROXO_REG_FREQ0, reg, ARRAY_SIZE(reg));
+	if (ret)
+		return ret;
+
+	ret = regmap_read(proxo->regmap, PROXO_REG_XO, &xo);
+	if (ret)
+		return ret;
+
+	*out_div = (u16_get_bits(reg[1], OUTDIV_8_MASK) << 8) + reg[0];
+	*fb_int = (u16_get_bits(reg[2], FBDIV_INT_8_7_MASK) << 7) + (reg[1] & FBDIV_INT_6_0_MASK);
+	*fb_frac = ((u32)reg[5] << 16) + ((u32)reg[4] << 8) + reg[3];
+	*double_dis = !!(xo & DOUBLE_DIS_MASK);
+
+	if (*fb_frac > (PROXO_FRAC_DIVISOR >> 1))
+		(*fb_int)--;
+
+	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u\n",
+		 __func__, *out_div, *fb_int, *fb_frac, *double_dis);
+
+	return ret;
+}
+
+static int proxo_get_defaults(struct clk_proxo *proxo)
+{
+	int ret;
+
+	ret = proxo_get_divs(proxo, &proxo->out_div, &proxo->fb_int, &proxo->fb_frac,
+			     &proxo->double_dis);
+	if (ret)
+		return ret;
+
+	proxo->fvco = proxo_calc_fvco(proxo->fxtal, proxo->double_dis, proxo->fb_int,
+				      proxo->fb_frac);
+	proxo->fout = div_u64(proxo->fvco, proxo->out_div);
+
+	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u, fvco: %llu, fout: %u\n",
+		 __func__, proxo->out_div, proxo->fb_int, proxo->fb_frac, proxo->double_dis,
+		 proxo->fvco, proxo->fout);
+
+	return ret;
+}
+
+static int proxo_calc_divs(unsigned long frequency, struct clk_proxo *proxo, u32 *fout,
+			   u16 *out_div, u16 *fb_int, u32 *fb_frac, u8 *double_dis)
+{
+	int i;
+	u8 doubler;
+	u16 out_div_start;
+	u32 fref;
+	u64 fvco;
+	bool found = false, allow_frac = false;
+
+	out_div_start = 1 + div64_u64(PROXO_FVCO_MIN, frequency);
+	doubler = proxo->fxtal <= PROXO_FDBL_MAX ? 2 : 1;
+	fref = proxo->fxtal * doubler;
+	*fout = (u32)max(PROXO_FOUT_MIN, min(PROXO_FOUT_MAX, (unsigned long)*fout));
+	*out_div = PROXO_OUTDIV_MIN;
+	*fb_int = PROXO_FB_MIN;
+	*fb_frac = 0;
+	*double_dis = doubler == 1 ? 1 : 0;
+
+retry:
+	for (i = out_div_start; i <= PROXO_OUTDIV_MAX; ++i) {
+		*out_div = i;
+		fvco = frequency * *out_div;
+		if (fvco > PROXO_FVCO_MAX) {
+			allow_frac = true;
+			goto retry;
+		}
+		*fb_int = div_u64_rem(fvco, fref, fb_frac);
+		if (*fb_frac == 0) {
+			found = true;
+			break;
+		}
+		if (allow_frac) {
+			*fb_frac = 1 + (u32)div_u64((u64)*fb_frac << PROXO_FRAC_BITS, fref);
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	if (fvco < PROXO_FVCO_MIN || fvco > PROXO_FVCO_MAX)
+		return -EINVAL;
+
+	fvco = ((u64)fref * *fb_int) + div_u64((u64)fref * *fb_frac, PROXO_FRAC_DIVISOR);
+	*fout = div_u64(fvco, *out_div);
+
+	return 0;
+}
+
+static int proxo_update_frequency(struct clk_proxo *proxo)
+{
+	enum proxo_pll_mode pll_mode;
+	u8 cp_value;
+	u16 fb_int;
+	u8 reg[6];
+
+	cp_value = proxo_get_cp_value(proxo->fvco);
+	pll_mode = proxo->fb_frac == 0 ? PLL_MODE_INT : PLL_MODE_FRAC;
+	fb_int = proxo->fb_frac > (PROXO_FRAC_DIVISOR >> 1) ? proxo->fb_int + 1 : proxo->fb_int;
+
+	reg[0] = proxo->out_div & 0xff;
+	reg[1] = ((proxo->out_div >> 1) & OUTDIV_8_MASK) + (fb_int & FBDIV_INT_6_0_MASK);
+	reg[2] = (fb_int >> 3) & FBDIV_INT_8_7_MASK;
+	reg[2] = u8_replace_bits(reg[2], cp_value, CP_MASK);
+	reg[2] = u8_replace_bits(reg[2], pll_mode, PLL_MODE_MASK);
+	reg[3] = proxo->fb_frac & 0xff;
+	reg[4] = (proxo->fb_frac >> 8) & 0xff;
+	reg[5] = (proxo->fb_frac >> 16) & 0xff;
+
+	return regmap_bulk_write(proxo->regmap, PROXO_REG_FREQ0, reg, ARRAY_SIZE(reg));
+}
+
+static int proxo_set_frequency(struct clk_proxo *proxo, unsigned long frequency)
+{
+	int ret;
+
+	ret = proxo_calc_divs(frequency, proxo, &proxo->fout, &proxo->out_div, &proxo->fb_int,
+			      &proxo->fb_frac, &proxo->double_dis);
+	if (ret)
+		return ret;
+
+	proxo->fvco = proxo_calc_fvco(proxo->fxtal, proxo->double_dis, proxo->fb_int,
+				      proxo->fb_frac);
+	proxo->fout = div_u64(proxo->fvco, proxo->out_div);
+
+	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u, fvco: %llu, fout: %u\n",
+		 __func__, proxo->out_div, proxo->fb_int, proxo->fb_frac,
+	proxo->double_dis, proxo->fvco, proxo->fout);
+
+	proxo_update_frequency(proxo);
+
+	/* trigger frequency change */
+	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x00);
+	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x01);
+	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x00);
+
+	return ret;
+}
+
+static unsigned long proxo_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_proxo *proxo = to_clk_proxo(hw);
+	int ret;
+	u8 double_dis;
+	u16 out_div, fb_int;
+	u32 fout, fb_frac;
+	u64 fvco;
+
+	ret = proxo_get_divs(proxo, &out_div, &fb_int, &fb_frac, &double_dis);
+	if (ret) {
+		dev_err(&proxo->i2c_client->dev, "unable to recalc rate\n");
+		return 0;
+	}
+
+	fvco = proxo_calc_fvco(proxo->fxtal, double_dis, fb_int, fb_frac);
+	fout = div_u64(fvco, out_div);
+
+	return fout;
+}
+
+static long proxo_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *parent_rate)
+{
+	struct clk_proxo *proxo = to_clk_proxo(hw);
+	int ret;
+	u8 double_dis;
+	u16 out_div, fb_int;
+	u32 fout, fb_frac;
+
+	if (!rate)
+		return 0;
+
+	ret = proxo_calc_divs(rate, proxo, &fout, &out_div, &fb_int, &fb_frac, &double_dis);
+	if (ret) {
+		dev_err(&proxo->i2c_client->dev, "unable to round rate\n");
+		return 0;
+	}
+
+	return fout;
+}
+
+static int proxo_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
+{
+	struct clk_proxo *proxo = to_clk_proxo(hw);
+
+	if (rate < PROXO_FOUT_MIN || rate > PROXO_FOUT_MAX) {
+		dev_err(&proxo->i2c_client->dev, "requested frequency %lu Hz is out of range\n",
+			rate);
+		return -EINVAL;
+	}
+
+	return proxo_set_frequency(proxo, rate);
+}
+
+static const struct clk_ops proxo_clk_ops = {
+	.recalc_rate = proxo_recalc_rate,
+	.round_rate = proxo_round_rate,
+	.set_rate = proxo_set_rate,
+};
+
+static const struct i2c_device_id proxo_i2c_id[] = {
+	{ "proxo-xp", PROXO_XP },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, proxo_i2c_id);
+
+static const struct regmap_config proxo_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x63,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_write = true,
+	.use_single_read = true,
+};
+
+static int proxo_probe(struct i2c_client *client)
+{
+	struct clk_proxo *proxo;
+	struct clk_init_data init;
+	const struct i2c_device_id *id = i2c_match_id(proxo_i2c_id, client);
+	int ret;
+
+	proxo = devm_kzalloc(&client->dev, sizeof(*proxo), GFP_KERNEL);
+	if (!proxo)
+		return -ENOMEM;
+
+	init.ops = &proxo_clk_ops;
+	init.flags = 0;
+	init.num_parents = 0;
+	proxo->hw.init = &init;
+	proxo->i2c_client = client;
+	proxo->model = id->driver_data;
+
+	if (of_property_read_string(client->dev.of_node, "clock-output-names", &init.name))
+		init.name = client->dev.of_node->name;
+
+	if (of_property_read_u32(client->dev.of_node, "renesas,crystal-frequency-hz", &proxo->fxtal))
+		proxo->fxtal = PROXO_DEFAULT_XTAL;
+
+	proxo->regmap = devm_regmap_init_i2c(client, &proxo_regmap_config);
+	if (IS_ERR(proxo->regmap))
+		return PTR_ERR(proxo->regmap);
+
+	i2c_set_clientdata(client, proxo);
+
+	ret = proxo_get_defaults(proxo);
+	if (ret) {
+		dev_err(&client->dev, "getting defaults failed\n");
+		return ret;
+	}
+
+	ret = devm_clk_hw_register(&client->dev, &proxo->hw);
+	if (ret) {
+		dev_err(&client->dev, "clock registration failed\n");
+		return ret;
+	}
+
+	ret = of_clk_add_hw_provider(client->dev.of_node, of_clk_hw_simple_get, &proxo->hw);
+	if (ret) {
+		dev_err(&client->dev, "unable to add clk provider\n");
+		return ret;
+	}
+
+	ret = clk_set_rate_range(proxo->hw.clk, PROXO_FOUT_MIN, PROXO_FOUT_MAX);
+	if (ret) {
+		dev_err(&client->dev, "clk_set_rate_range failed\n");
+		return ret;
+	}
+
+	dev_info(&client->dev, "registered, current frequency %u Hz\n", proxo->fout);
+
+	return ret;
+}
+
+static int proxo_remove(struct i2c_client *client)
+{
+	of_clk_del_provider(client->dev.of_node);
+	return 0;
+}
+
+static const struct of_device_id proxo_of_match[] = {
+	{ .compatible = "renesas,proxo-xp" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, proxo_of_match);
+
+static struct i2c_driver proxo_i2c_driver = {
+	.driver = {
+		.name = "proxo",
+		.of_match_table = proxo_of_match,
+	},
+	.probe_new = proxo_probe,
+	.remove = proxo_remove,
+	.id_table = proxo_i2c_id,
+};
+module_i2c_driver(proxo_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alex Helms <alexander.helms.jy@renesas.com");
+MODULE_DESCRIPTION("Renesas ProXO common clock framework driver");
-- 
2.30.2

Re: [PATCH v3 2/2] clk: Add support for Renesas ProXO oscillator

[Krzysztof Kozlowski]

On 16/11/2022 00:37, Alex Helms wrote:
> ProXO is a programmable ultra-low phase noise quartz-based
> oscillator with a single output. This driver supports changing
> the frequency of the ProXP XP variant.
> 
> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> ---
>  MAINTAINERS             |   1 +
>  drivers/clk/Kconfig     |   7 +
>  drivers/clk/Makefile    |   1 +
>  drivers/clk/clk-proxo.c | 410 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 419 insertions(+)
>  create mode 100644 drivers/clk/clk-proxo.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d52a8a5d2..c7f5c0655 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16084,6 +16084,7 @@ RENESAS PROXO CLOCK DRIVER
>  M:	Alex Helms <alexander.helms.jy@renesas.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> +F:	drivers/clk/clk-proxo.c
>  
>  RESET CONTROLLER FRAMEWORK
>  M:	Philipp Zabel <p.zabel@pengutronix.de>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 8f905df60..31f684d69 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -396,6 +396,13 @@ config COMMON_CLK_K210
>  	help
>  	  Support for the Canaan Kendryte K210 RISC-V SoC clocks.
>  
> +config COMMON_CLK_PROXO
> +	bool "Clock driver for Renesas ProXO"
> +	depends on I2C && OF
> +	select REGMAP_I2C
> +	help
> +	  Support for the Renesas ProXO oscillator clock.
> +
>  source "drivers/clk/actions/Kconfig"
>  source "drivers/clk/analogbits/Kconfig"
>  source "drivers/clk/baikal-t1/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 99941b4a3..be6e28cc4 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -52,6 +52,7 @@ 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_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
> +obj-$(CONFIG_COMMON_CLK_PROXO)		+= clk-proxo.o
>  obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
>  obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
>  obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
> diff --git a/drivers/clk/clk-proxo.c b/drivers/clk/clk-proxo.c
> new file mode 100644
> index 000000000..7d4268687
> --- /dev/null
> +++ b/drivers/clk/clk-proxo.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Common clock framework driver for the ProXO family of quartz-based oscillators.
> + *
> + * 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/swab.h>
> +
> +/* Most ProXO products have a 50MHz xtal, can be overridden in device tree */
> +#define PROXO_DEFAULT_XTAL	50000000
> +
> +/* VCO range is 6.86 GHz to 8.65 GHz */
> +#define PROXO_FVCO_MIN		6860000000ULL
> +#define PROXO_FVCO_MAX		8650000000ULL
> +
> +/* Output range is 15MHz to 2.1GHz */
> +#define PROXO_FOUT_MIN		15000000UL
> +#define PROXO_FOUT_MAX		2100000000UL
> +
> +#define PROXO_FRAC_BITS		24
> +#define PROXO_FRAC_DIVISOR	BIT(PROXO_FRAC_BITS)
> +
> +/* Disable the doubler if the crystal is > 80MHz */
> +#define PROXO_FDBL_MAX		80000000U
> +
> +#define PROXO_OUTDIV_MIN	4
> +#define PROXO_OUTDIV_MAX	511
> +#define PROXO_FB_MIN		41
> +
> +#define PROXO_REG_FREQ0		0x10
> +#define PROXO_REG_XO		0x51
> +#define PROXO_REG_TRIG		0x62
> +
> +#define OUTDIV_8_MASK		0x80
> +#define FBDIV_INT_8_7_MASK	0x30
> +#define FBDIV_INT_6_0_MASK	0x7f
> +#define DOUBLE_DIS_MASK		0x80
> +#define CP_MASK			0x0e
> +#define PLL_MODE_MASK		0x01
> +
> +enum proxo_model {
> +	PROXO_XP,
> +};
> +
> +enum proxo_pll_mode {
> +	PLL_MODE_FRAC,
> +	PLL_MODE_INT,
> +};
> +
> +struct clk_proxo {
> +	struct clk_hw hw;
> +	struct regmap *regmap;
> +	struct i2c_client *i2c_client;
> +	enum proxo_model model;
> +	u32 fxtal;
> +	u64 fvco;
> +	u32 fout;
> +	u8 double_dis;
> +	u16 fb_int;
> +	u32 fb_frac;
> +	u16 out_div;
> +};
> +
> +#define to_clk_proxo(_hw)	container_of(_hw, struct clk_proxo, hw)
> +
> +static u8 proxo_get_cp_value(u64 fvco)
> +{
> +	if (fvco < 7000000000ULL)
> +		return 5;
> +	else if (fvco >= 7000000000ULL && fvco < 7400000000ULL)
> +		return 4;
> +	else if (fvco >= 7400000000ULL && fvco < 7800000000ULL)
> +		return 3;
> +	else
> +		return 2;
> +}
> +
> +static u64 proxo_calc_fvco(u32 fxtal, u8 double_dis, u16 fb_int, u32 fb_frac)
> +{
> +	u64 fref, fvco;
> +	u8 doubler;
> +
> +	doubler = double_dis ? 1 : 2;
> +	fref = (u64)fxtal * doubler;
> +	fvco = (fref * fb_int) + div_u64(fref * fb_frac, PROXO_FRAC_DIVISOR);
> +
> +	return fvco;
> +}
> +
> +static int proxo_get_divs(struct clk_proxo *proxo, u16 *out_div, u16 *fb_int, u32 *fb_frac,
> +			  u8 *double_dis)
> +{
> +	int ret;
> +	u8 reg[6];
> +	unsigned int xo;
> +
> +	ret = regmap_bulk_read(proxo->regmap, PROXO_REG_FREQ0, reg, ARRAY_SIZE(reg));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(proxo->regmap, PROXO_REG_XO, &xo);
> +	if (ret)
> +		return ret;
> +
> +	*out_div = (u16_get_bits(reg[1], OUTDIV_8_MASK) << 8) + reg[0];
> +	*fb_int = (u16_get_bits(reg[2], FBDIV_INT_8_7_MASK) << 7) + (reg[1] & FBDIV_INT_6_0_MASK);
> +	*fb_frac = ((u32)reg[5] << 16) + ((u32)reg[4] << 8) + reg[3];
> +	*double_dis = !!(xo & DOUBLE_DIS_MASK);
> +
> +	if (*fb_frac > (PROXO_FRAC_DIVISOR >> 1))
> +		(*fb_int)--;
> +
> +	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u\n",
> +		 __func__, *out_div, *fb_int, *fb_frac, *double_dis);

Use dev_dbg.

> +
> +	return ret;
> +}
> +
> +static int proxo_get_defaults(struct clk_proxo *proxo)
> +{
> +	int ret;
> +
> +	ret = proxo_get_divs(proxo, &proxo->out_div, &proxo->fb_int, &proxo->fb_frac,
> +			     &proxo->double_dis);
> +	if (ret)
> +		return ret;
> +
> +	proxo->fvco = proxo_calc_fvco(proxo->fxtal, proxo->double_dis, proxo->fb_int,
> +				      proxo->fb_frac);
> +	proxo->fout = div_u64(proxo->fvco, proxo->out_div);
> +
> +	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u, fvco: %llu, fout: %u\n",
> +		 __func__, proxo->out_div, proxo->fb_int, proxo->fb_frac, proxo->double_dis,
> +		 proxo->fvco, proxo->fout);

Use dev_dbg.

> +
> +	return ret;
> +}
> +
> +static int proxo_calc_divs(unsigned long frequency, struct clk_proxo *proxo, u32 *fout,
> +			   u16 *out_div, u16 *fb_int, u32 *fb_frac, u8 *double_dis)
> +{
> +	int i;
> +	u8 doubler;
> +	u16 out_div_start;
> +	u32 fref;
> +	u64 fvco;
> +	bool found = false, allow_frac = false;
> +
> +	out_div_start = 1 + div64_u64(PROXO_FVCO_MIN, frequency);
> +	doubler = proxo->fxtal <= PROXO_FDBL_MAX ? 2 : 1;
> +	fref = proxo->fxtal * doubler;
> +	*fout = (u32)max(PROXO_FOUT_MIN, min(PROXO_FOUT_MAX, (unsigned long)*fout));
> +	*out_div = PROXO_OUTDIV_MIN;
> +	*fb_int = PROXO_FB_MIN;
> +	*fb_frac = 0;
> +	*double_dis = doubler == 1 ? 1 : 0;
> +
> +retry:
> +	for (i = out_div_start; i <= PROXO_OUTDIV_MAX; ++i) {
> +		*out_div = i;
> +		fvco = frequency * *out_div;
> +		if (fvco > PROXO_FVCO_MAX) {
> +			allow_frac = true;
> +			goto retry;
> +		}
> +		*fb_int = div_u64_rem(fvco, fref, fb_frac);
> +		if (*fb_frac == 0) {
> +			found = true;
> +			break;
> +		}
> +		if (allow_frac) {
> +			*fb_frac = 1 + (u32)div_u64((u64)*fb_frac << PROXO_FRAC_BITS, fref);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		return -EINVAL;
> +
> +	if (fvco < PROXO_FVCO_MIN || fvco > PROXO_FVCO_MAX)
> +		return -EINVAL;
> +
> +	fvco = ((u64)fref * *fb_int) + div_u64((u64)fref * *fb_frac, PROXO_FRAC_DIVISOR);
> +	*fout = div_u64(fvco, *out_div);
> +
> +	return 0;
> +}
> +
> +static int proxo_update_frequency(struct clk_proxo *proxo)
> +{
> +	enum proxo_pll_mode pll_mode;
> +	u8 cp_value;
> +	u16 fb_int;
> +	u8 reg[6];
> +
> +	cp_value = proxo_get_cp_value(proxo->fvco);
> +	pll_mode = proxo->fb_frac == 0 ? PLL_MODE_INT : PLL_MODE_FRAC;
> +	fb_int = proxo->fb_frac > (PROXO_FRAC_DIVISOR >> 1) ? proxo->fb_int + 1 : proxo->fb_int;
> +
> +	reg[0] = proxo->out_div & 0xff;
> +	reg[1] = ((proxo->out_div >> 1) & OUTDIV_8_MASK) + (fb_int & FBDIV_INT_6_0_MASK);
> +	reg[2] = (fb_int >> 3) & FBDIV_INT_8_7_MASK;
> +	reg[2] = u8_replace_bits(reg[2], cp_value, CP_MASK);
> +	reg[2] = u8_replace_bits(reg[2], pll_mode, PLL_MODE_MASK);
> +	reg[3] = proxo->fb_frac & 0xff;
> +	reg[4] = (proxo->fb_frac >> 8) & 0xff;
> +	reg[5] = (proxo->fb_frac >> 16) & 0xff;
> +
> +	return regmap_bulk_write(proxo->regmap, PROXO_REG_FREQ0, reg, ARRAY_SIZE(reg));
> +}
> +
> +static int proxo_set_frequency(struct clk_proxo *proxo, unsigned long frequency)
> +{
> +	int ret;
> +
> +	ret = proxo_calc_divs(frequency, proxo, &proxo->fout, &proxo->out_div, &proxo->fb_int,
> +			      &proxo->fb_frac, &proxo->double_dis);
> +	if (ret)
> +		return ret;
> +
> +	proxo->fvco = proxo_calc_fvco(proxo->fxtal, proxo->double_dis, proxo->fb_int,
> +				      proxo->fb_frac);
> +	proxo->fout = div_u64(proxo->fvco, proxo->out_div);
> +
> +	pr_debug("%s - out_div: %u, fb_int: %u, fb_frac: %u, doubler_dis: %u, fvco: %llu, fout: %u\n",
> +		 __func__, proxo->out_div, proxo->fb_int, proxo->fb_frac,
> +	proxo->double_dis, proxo->fvco, proxo->fout);

Wrong indent.

Use dev_dbg.

> +
> +	proxo_update_frequency(proxo);
> +
> +	/* trigger frequency change */
> +	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x00);
> +	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x01);
> +	regmap_write(proxo->regmap, PROXO_REG_TRIG, 0x00);
> +
> +	return ret;
> +}
> +
> +static unsigned long proxo_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> +	struct clk_proxo *proxo = to_clk_proxo(hw);
> +	int ret;
> +	u8 double_dis;
> +	u16 out_div, fb_int;
> +	u32 fout, fb_frac;
> +	u64 fvco;
> +
> +	ret = proxo_get_divs(proxo, &out_div, &fb_int, &fb_frac, &double_dis);
> +	if (ret) {
> +		dev_err(&proxo->i2c_client->dev, "unable to recalc rate\n");
> +		return 0;
> +	}
> +
> +	fvco = proxo_calc_fvco(proxo->fxtal, double_dis, fb_int, fb_frac);
> +	fout = div_u64(fvco, out_div);
> +
> +	return fout;
> +}
> +
> +static long proxo_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *parent_rate)
> +{
> +	struct clk_proxo *proxo = to_clk_proxo(hw);
> +	int ret;
> +	u8 double_dis;
> +	u16 out_div, fb_int;
> +	u32 fout, fb_frac;
> +
> +	if (!rate)
> +		return 0;
> +
> +	ret = proxo_calc_divs(rate, proxo, &fout, &out_div, &fb_int, &fb_frac, &double_dis);
> +	if (ret) {
> +		dev_err(&proxo->i2c_client->dev, "unable to round rate\n");
> +		return 0;
> +	}
> +
> +	return fout;
> +}
> +
> +static int proxo_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> +{
> +	struct clk_proxo *proxo = to_clk_proxo(hw);
> +
> +	if (rate < PROXO_FOUT_MIN || rate > PROXO_FOUT_MAX) {
> +		dev_err(&proxo->i2c_client->dev, "requested frequency %lu Hz is out of range\n",
> +			rate);
> +		return -EINVAL;
> +	}
> +
> +	return proxo_set_frequency(proxo, rate);
> +}
> +
> +static const struct clk_ops proxo_clk_ops = {
> +	.recalc_rate = proxo_recalc_rate,
> +	.round_rate = proxo_round_rate,
> +	.set_rate = proxo_set_rate,
> +};
> +
> +static const struct i2c_device_id proxo_i2c_id[] = {
> +	{ "proxo-xp", PROXO_XP },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, proxo_i2c_id);
> +
> +static const struct regmap_config proxo_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x63,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_write = true,
> +	.use_single_read = true,
> +};
> +
> +static int proxo_probe(struct i2c_client *client)
> +{
> +	struct clk_proxo *proxo;
> +	struct clk_init_data init;
> +	const struct i2c_device_id *id = i2c_match_id(proxo_i2c_id, client);
> +	int ret;
> +
> +	proxo = devm_kzalloc(&client->dev, sizeof(*proxo), GFP_KERNEL);
> +	if (!proxo)
> +		return -ENOMEM;
> +
> +	init.ops = &proxo_clk_ops;
> +	init.flags = 0;
> +	init.num_parents = 0;
> +	proxo->hw.init = &init;
> +	proxo->i2c_client = client;
> +	proxo->model = id->driver_data;
> +
> +	if (of_property_read_string(client->dev.of_node, "clock-output-names", &init.name))
> +		init.name = client->dev.of_node->name;
> +
> +	if (of_property_read_u32(client->dev.of_node, "renesas,crystal-frequency-hz", &proxo->fxtal))
> +		proxo->fxtal = PROXO_DEFAULT_XTAL;
> +
> +	proxo->regmap = devm_regmap_init_i2c(client, &proxo_regmap_config);
> +	if (IS_ERR(proxo->regmap))
> +		return PTR_ERR(proxo->regmap);
> +
> +	i2c_set_clientdata(client, proxo);
> +
> +	ret = proxo_get_defaults(proxo);
> +	if (ret) {
> +		dev_err(&client->dev, "getting defaults failed\n");

dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = devm_clk_hw_register(&client->dev, &proxo->hw);
> +	if (ret) {
> +		dev_err(&client->dev, "clock registration failed\n");

dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = of_clk_add_hw_provider(client->dev.of_node, of_clk_hw_simple_get, &proxo->hw);
> +	if (ret) {
> +		dev_err(&client->dev, "unable to add clk provider\n");

dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = clk_set_rate_range(proxo->hw.clk, PROXO_FOUT_MIN, PROXO_FOUT_MAX);
> +	if (ret) {
> +		dev_err(&client->dev, "clk_set_rate_range failed\n");
> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "registered, current frequency %u Hz\n", proxo->fout);

No silly probe confirmations.


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO

[Krzysztof Kozlowski]

On 16/11/2022 00:37, Alex Helms wrote:
> Add dt bindings for the Renesas ProXO oscillator.
> 
> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> ---
>  .../bindings/clock/renesas,proxo.yaml         | 51 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,proxo.yaml b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> new file mode 100644
> index 000000000..ff960196d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,proxo.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas ProXO Oscillator Device Tree Bindings

Same comments as for your other patch. All the same...

> +
> +maintainers:
> +  - Alex Helms <alexander.helms.jy@renesas.com>
> +
> +description:
> +  Renesas ProXO is a family of programmable ultra-low phase noise
> +  quartz-based oscillators.
> +
> +properties:
> +  '#clock-cells':
> +    const: 0
> +
> +  compatible:
> +    enum:
> +      - renesas,proxo-xp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  renesas,crystal-frequency-hz:
> +    description: Internal crystal frequency, default is 50000000 (50MHz)

If it is internal, then it is fixed, right? Embedded in the chip, always
the same. Why do you need to specify it?


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO

[Krzysztof Kozlowski]

On 16/11/2022 09:50, Krzysztof Kozlowski wrote:

>> +
>> +  renesas,crystal-frequency-hz:
>> +    description: Internal crystal frequency, default is 50000000 (50MHz)
> 
> If it is internal, then it is fixed, right? Embedded in the chip, always
> the same. Why do you need to specify it?
> 

BTW, this ignores Rob's comment, so go back to it.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO

[Alex Helms]

On 11/16/2022 1:50 AM, Krzysztof Kozlowski wrote:
> On 16/11/2022 00:37, Alex Helms wrote:
>> Add dt bindings for the Renesas ProXO oscillator.
>>
>> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
>> ---
>>  .../bindings/clock/renesas,proxo.yaml         | 51 +++++++++++++++++++
>>  MAINTAINERS                                   |  5 ++
>>  2 files changed, 56 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,proxo.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/renesas,proxo.yaml b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
>> new file mode 100644
>> index 000000000..ff960196d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
>> @@ -0,0 +1,51 @@
>> +# 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%2Cproxo.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=iGbtWJLjV%2FM%2Fps0lPk7f40bMzX8qdt8VZBtH9J4LdOw%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%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=zYh4aHuw6G6A35rXBD7FTKeFrC7Hfcxag60ghkKUaGA%3D&amp;reserved=0
>> +
>> +title: Renesas ProXO Oscillator Device Tree Bindings
> 
> Same comments as for your other patch. All the same...
> 
>> +
>> +maintainers:
>> +  - Alex Helms <alexander.helms.jy@renesas.com>
>> +
>> +description:
>> +  Renesas ProXO is a family of programmable ultra-low phase noise
>> +  quartz-based oscillators.
>> +
>> +properties:
>> +  '#clock-cells':
>> +    const: 0
>> +
>> +  compatible:
>> +    enum:
>> +      - renesas,proxo-xp
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clock-output-names:
>> +    maxItems: 1
>> +
>> +  renesas,crystal-frequency-hz:
>> +    description: Internal crystal frequency, default is 50000000 (50MHz)
> 
> If it is internal, then it is fixed, right? Embedded in the chip, always
> the same. Why do you need to specify it?
> 

Yes, it is embedded in the package but there are different values
depending on what chip is ordered and therefore must be specified for
some configurations.

I'm also not sure what you mean by me ignoring Rob's comment. I
explained my case for calling it "crystal-frequency-hz" and moved
forward. I can call it "clock-frequency" if you want but I find that
more confusing. Yes it is a built-in name in the schema but it seems to
be used in a variety of ways. Some devices use it as a crystal input,
but most seem to use it as the desired output frequency of the device
which is not how it is used here. Therefore I chose a more clear name
that better reflects what it is doing.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO

[Rob Herring]

On Wed, Nov 16, 2022 at 01:17:54PM -0700, Alex Helms wrote:
> On 11/16/2022 1:50 AM, Krzysztof Kozlowski wrote:
> > On 16/11/2022 00:37, Alex Helms wrote:
> >> Add dt bindings for the Renesas ProXO oscillator.
> >>
> >> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> >> ---
> >>  .../bindings/clock/renesas,proxo.yaml         | 51 +++++++++++++++++++
> >>  MAINTAINERS                                   |  5 ++
> >>  2 files changed, 56 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/renesas,proxo.yaml b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >> new file mode 100644
> >> index 000000000..ff960196d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >> @@ -0,0 +1,51 @@
> >> +# 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%2Cproxo.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=iGbtWJLjV%2FM%2Fps0lPk7f40bMzX8qdt8VZBtH9J4LdOw%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%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=zYh4aHuw6G6A35rXBD7FTKeFrC7Hfcxag60ghkKUaGA%3D&amp;reserved=0
> >> +
> >> +title: Renesas ProXO Oscillator Device Tree Bindings
> > 
> > Same comments as for your other patch. All the same...
> > 
> >> +
> >> +maintainers:
> >> +  - Alex Helms <alexander.helms.jy@renesas.com>
> >> +
> >> +description:
> >> +  Renesas ProXO is a family of programmable ultra-low phase noise
> >> +  quartz-based oscillators.
> >> +
> >> +properties:
> >> +  '#clock-cells':
> >> +    const: 0
> >> +
> >> +  compatible:
> >> +    enum:
> >> +      - renesas,proxo-xp
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clock-output-names:
> >> +    maxItems: 1
> >> +
> >> +  renesas,crystal-frequency-hz:
> >> +    description: Internal crystal frequency, default is 50000000 (50MHz)
> > 
> > If it is internal, then it is fixed, right? Embedded in the chip, always
> > the same. Why do you need to specify it?
> > 
> 
> Yes, it is embedded in the package but there are different values
> depending on what chip is ordered and therefore must be specified for
> some configurations.
> 
> I'm also not sure what you mean by me ignoring Rob's comment. I
> explained my case for calling it "crystal-frequency-hz" and moved
> forward. I can call it "clock-frequency" if you want but I find that
> more confusing. Yes it is a built-in name in the schema but it seems to
> be used in a variety of ways. Some devices use it as a crystal input,
> but most seem to use it as the desired output frequency of the device
> which is not how it is used here. Therefore I chose a more clear name
> that better reflects what it is doing.

I think it is fine as-is. But you should have 'default: 50000000' 
instead of prose.

Rob