[PATCH v2 0/3] Binding and driver for voltage controlled oscillators

[PATCH v2 0/3] Binding and driver for voltage controlled oscillators

[Heiko Stuebner]

Rockchip boards with PCIe3 controllers inside the soc (rk3568, rk3588) have
external oscillators on the board to generate the needed 100MHz reference
clock the PCIe3 controller needs.

Often these clock generators need supplies to be enabled to run.

Modelling this clock has taken a number of shapes:
- The rk3568 Rock-3a modelled the generator-regulator as "phy-supply" [0]
  &pcie30phy {
  	phy-supply = <&vcc3v3_pi6c_03>;
  	status = "okay";
  };
  which is of course not part of the binding

- On the Rock-5-ITX the supply of the clock generator is controlled by
  the same gpio as the regulator supplying the the port connected to the
  pcie30x4 controller, so if this controller probes first, both
  controllers will just run. But if the pcie30x2 controller probes first
  (which has a different supply), the controller will stall at the first
  dbi read.

There are other types too, where an 25MHz oscillator supplies a PLL
chip like the diodes,pi6c557 used on Theobroma Jaguar and Tiger boards.

As we established in v1 [1], these are essentially different types, so
this series attempts to solve the first case of "voltage controlled
oscillators" as Stephen called them.

changes in v2:
- drop the Diodes PLLs for now, to get the first variant right
- rename stuff to voltage-oscillator / clk_vco as suggested by Stephen
- require vdd-supply in the binding
- enable-gpios stays optional, as they often are tied to vdd-supply
- drop deprecated elements that were left in from the fixed clock binding


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts#n605
[1] https://lore.kernel.org/linux-clk/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/

Heiko Stuebner (3):
  dt-bindings: clocks: add binding for voltage-controlled-oscillators
  clk: add driver for voltage controlled oscillators
  arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

 .../bindings/clock/voltage-oscillator.yaml    |  49 +++++++
 .../boot/dts/rockchip/rk3588-rock-5-itx.dts   |  38 ++++-
 drivers/clk/Kconfig                           |  10 ++
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-vco.c                         | 133 ++++++++++++++++++
 5 files changed, 229 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
 create mode 100644 drivers/clk/clk-vco.c

-- 
2.39.2

[PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stuebner]

In contrast to fixed clocks that are described as ungateable, boards
sometimes use additional oscillators for things like PCIe reference
clocks, that need actual supplies to get enabled and enable-gpios to be
toggled for them to work.

This adds a binding for such oscillators that are not configurable
themself, but need to handle supplies for them to work.

In schematics they often can be seen as

         ----------------
Enable - | 100MHz,3.3V, | - VDD
         |    3225      |
   GND - |              | - OUT
         ----------------

or similar. The enable pin might be separate but can also just be tied
to the vdd supply, hence it is optional in the binding.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml

diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
new file mode 100644
index 0000000000000..8bff6b0fd582e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Voltage controlled oscillator
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+properties:
+  compatible:
+    const: voltage-oscillator
+
+  "#clock-cells":
+    const: 0
+
+  clock-frequency: true
+
+  clock-output-names:
+    maxItems: 1
+
+  enable-gpios:
+    description:
+      Contains a single GPIO specifier for the GPIO that enables and disables
+      the oscillator.
+    maxItems: 1
+
+  vdd-supply:
+    description: handle of the regulator that provides the supply voltage
+
+required:
+  - compatible
+  - "#clock-cells"
+  - clock-frequency
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    voltage-oscillator {
+      compatible = "voltage-oscillator";
+      #clock-cells = <0>;
+      clock-frequency = <1000000000>;
+      vdd-supply = <&reg_vdd>;
+    };
+...
-- 
2.39.2

[PATCH v2 2/3] clk: add driver for voltage controlled oscillators

[Heiko Stuebner]

In contrast to fixed clocks that are described as ungateable, boards
sometimes use additional oscillators for things like PCIe reference
clocks, that need actual supplies to get enabled and enable-gpios to be
toggled for them to work.

This adds a driver for those generic voltage controlled oscillators,
that can show up in schematics looking like

         ----------------
Enable - | 100MHz,3.3V, | - VDD
         |    3225      |
   GND - |              | - OUT
         ----------------

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/Kconfig   |  10 ++++
 drivers/clk/Makefile  |   1 +
 drivers/clk/clk-vco.c | 133 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 drivers/clk/clk-vco.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 3e9099504fad4..e93a380b6ee47 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -414,6 +414,16 @@ 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_VCO
+	tristate "Clock driver for voltage controlled oscillators"
+	depends on GPIOLIB && REGULATOR
+	help
+	  This driver supports generic voltage controlled oscillators that
+	  are not configurable but need supplies to be enabled to run.
+	  Generally they need a supply voltage to be enabled and may also
+	  require a separate enable pin, though in a lot of cases, vdd
+	  and enable control might be tied to the same supply.
+
 config COMMON_CLK_STM32F
 	def_bool COMMON_CLK && (MACH_STM32F429 || MACH_STM32F469 || MACH_STM32F746)
 	help
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 4abe16c8ccdfe..ca7b7b7ddfd8d 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_COMMON_CLK_SI521XX)	+= clk-si521xx.o
 obj-$(CONFIG_COMMON_CLK_VC3)		+= clk-versaclock3.o
 obj-$(CONFIG_COMMON_CLK_VC5)		+= clk-versaclock5.o
 obj-$(CONFIG_COMMON_CLK_VC7)		+= clk-versaclock7.o
+obj-$(CONFIG_COMMON_CLK_VCO)		+= clk-vco.o
 obj-$(CONFIG_COMMON_CLK_WM831X)		+= clk-wm831x.o
 obj-$(CONFIG_COMMON_CLK_XGENE)		+= clk-xgene.o
 
diff --git a/drivers/clk/clk-vco.c b/drivers/clk/clk-vco.c
new file mode 100644
index 0000000000000..f7fe2bc627f36
--- /dev/null
+++ b/drivers/clk/clk-vco.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ *
+ * Generic voltage controlled oscillator
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct clk_vco {
+	struct device *dev;
+	struct clk_hw hw;
+	u32 rate;
+	struct regulator *supply;
+	struct gpio_desc *enable_gpio;
+};
+
+#define to_clk_vco(_hw) container_of(_hw, struct clk_vco, hw)
+
+static int clk_vco_prepare(struct clk_hw *hw)
+{
+	return regulator_enable(to_clk_vco(hw)->supply);
+}
+
+static void clk_vco_unprepare(struct clk_hw *hw)
+{
+	regulator_disable(to_clk_vco(hw)->supply);
+}
+
+static int clk_vco_enable(struct clk_hw *hw)
+{
+	gpiod_set_value(to_clk_vco(hw)->enable_gpio, 1);
+	return 0;
+}
+
+static void clk_vco_disable(struct clk_hw *hw)
+{
+	gpiod_set_value(to_clk_vco(hw)->enable_gpio, 0);
+}
+
+static unsigned long clk_vco_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	return to_clk_vco(hw)->rate;
+}
+
+const struct clk_ops clk_vco_ops = {
+	.prepare = clk_vco_prepare,
+	.unprepare = clk_vco_unprepare,
+	.enable = clk_vco_enable,
+	.disable = clk_vco_disable,
+	.recalc_rate = clk_vco_recalc_rate,
+};
+
+static int clk_vco_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_vco *clkgen;
+	const char *clk_name;
+	int ret;
+
+	clkgen = devm_kzalloc(dev, sizeof(*clkgen), GFP_KERNEL);
+	if (!clkgen)
+		return -ENOMEM;
+
+	clkgen->dev = dev;
+
+	if (device_property_read_u32(dev, "clock-frequency", &clkgen->rate))
+		return dev_err_probe(dev, -EIO, "failed to get clock-frequency");
+
+	ret = device_property_read_string(dev, "clock-output-names", &clk_name);
+	if (ret)
+		clk_name = fwnode_get_name(dev->fwnode);
+
+	clkgen->supply = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(clkgen->supply)) {
+		if (PTR_ERR(clkgen->supply) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(clkgen->supply),
+					     "failed to get regulator\n");
+		clkgen->supply = NULL;
+	}
+
+	clkgen->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(clkgen->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(clkgen->enable_gpio),
+				     "failed to get gpio\n");
+
+	ret = gpiod_direction_output(clkgen->enable_gpio, 0);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to set gpio output");
+
+	clkgen->hw.init = CLK_HW_INIT_NO_PARENT(clk_name, &clk_vco_ops, 0);
+
+	/* register the clock */
+	ret = devm_clk_hw_register(dev, &clkgen->hw);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register clock\n");
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					  &clkgen->hw);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register clock provider\n");
+
+	return 0;
+}
+
+static const struct of_device_id clk_vco_ids[] = {
+	{ .compatible = "voltage-oscillator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_vco_ids);
+
+static struct platform_driver clk_vco_driver = {
+	.driver = {
+		.name = "clk_vco",
+		.of_match_table = clk_vco_ids,
+	},
+	.probe = clk_vco_probe,
+};
+module_platform_driver(clk_vco_driver);
+
+MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
+MODULE_DESCRIPTION("Voltage controlled oscillator driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

[PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Heiko Stuebner]

The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and its
SATA controller with 2 lanes each. The supply for the refclk oscillator is
the same that supplies the M.2 slot, but the SATA controller port is
supplied by a different rail.

This leads to the effect that if the PCIe30x4 controller for the M.2
probes first, everything works normally. But if the PCIe30x2 controller
that is connected to the SATA controller probes first, it will hang on
the first DBI read as nothing will have enabled the refclock before.

Fix this by describing the clock generator with its supplies so that
both controllers can reference it as needed.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../boot/dts/rockchip/rk3588-rock-5-itx.dts   | 38 ++++++++++++++++++-
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
index d0b922b8d67e8..37bc53f2796fc 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
@@ -72,6 +72,15 @@ hdd-led2 {
 		};
 	};
 
+	/* Unnamed voltage oscillator: 100MHz,3.3V,3225 */
+	pcie30_port0_refclk: pcie30_port1_refclk: pcie-voltage-oscillator {
+		compatible = "voltage-oscillator";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+		clock-output-names = "pcie30_refclk";
+		vdd-supply = <&vcc3v3_pi6c_05>;
+	};
+
 	fan0: pwm-fan {
 		compatible = "pwm-fan";
 		#cooling-cells = <2>;
@@ -146,13 +155,14 @@ vcc3v3_lan: vcc3v3_lan_phy2: regulator-vcc3v3-lan {
 		vin-supply = <&vcc_3v3_s3>;
 	};
 
-	vcc3v3_mkey: regulator-vcc3v3-mkey {
+	/* The PCIE30x4_PWREN_H controls two regulators */
+	vcc3v3_mkey: vcc3v3_pi6c_05: regulator-vcc3v3-pi6c-05 {
 		compatible = "regulator-fixed";
 		enable-active-high;
 		gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&pcie30x4_pwren_h>;
-		regulator-name = "vcc3v3_mkey";
+		regulator-name = "vcc3v3_pi6c_05";
 		regulator-min-microvolt = <3300000>;
 		regulator-max-microvolt = <3300000>;
 		startup-delay-us = <5000>;
@@ -513,6 +523,18 @@ &pcie30phy {
 
 /* ASMedia ASM1164 Sata controller */
 &pcie3x2 {
+	/*
+	 * The board has a "pcie_refclk" oscillator that needs enabling,
+	 * so add it to the list of clocks.
+	 */
+	clocks = <&cru ACLK_PCIE_2L_MSTR>, <&cru ACLK_PCIE_2L_SLV>,
+		 <&cru ACLK_PCIE_2L_DBI>, <&cru PCLK_PCIE_2L>,
+		 <&cru CLK_PCIE_AUX1>, <&cru CLK_PCIE2L_PIPE>,
+		 <&pcie30_port1_refclk>;
+	clock-names = "aclk_mst", "aclk_slv",
+		      "aclk_dbi", "pclk",
+		      "aux", "pipe",
+		      "ref";
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie30x2_perstn_m1_l>;
 	reset-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
@@ -522,6 +544,18 @@ &pcie3x2 {
 
 /* M.2 M.key */
 &pcie3x4 {
+	/*
+	 * The board has a "pcie_refclk" oscillator that needs enabling,
+	 * so add it to the list of clocks.
+	 */
+	clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
+		 <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
+		 <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>,
+		 <&pcie30_port0_refclk>;
+	clock-names = "aclk_mst", "aclk_slv",
+		      "aclk_dbi", "pclk",
+		      "aux", "pipe",
+		      "ref";
 	num-lanes = <2>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie30x4_perstn_m1_l>;
-- 
2.39.2

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Heiko,

Please see some comments below.

On 2024-07-15 13:02, Heiko Stuebner wrote:
> In contrast to fixed clocks that are described as ungateable, boards
> sometimes use additional oscillators for things like PCIe reference
> clocks, that need actual supplies to get enabled and enable-gpios to be
> toggled for them to work.
> 
> This adds a binding for such oscillators that are not configurable
> themself, but need to handle supplies for them to work.
> 
> In schematics they often can be seen as
> 
>          ----------------
> Enable - | 100MHz,3.3V, | - VDD
>          |    3225      |
>    GND - |              | - OUT
>          ----------------
> 
> or similar. The enable pin might be separate but can also just be tied
> to the vdd supply, hence it is optional in the binding.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> 
> diff --git
> a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> new file mode 100644
> index 0000000000000..8bff6b0fd582e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Voltage controlled oscillator

Frankly, I find the "voltage-oscillator" and "voltage controlled
oscillator" names awkward.  In general, "clock" is used throughout
the entire kernel, when it comes to naming files and defining
"compatible" strings.  Thus, I'd suggest that "clock" is used here
instead of "oscillator", because it's consistent and shorter.

How about using "gated-clock" for the "compatible" string, and
"Simple gated clock generator" instead of "voltage controlled
oscillator"?  Besides sounding awkward, "voltage controlled
oscillator" may suggest that the clock generator can be adjusted
or programmed somehow by applying the voltage, while it can only
be enabled or disabled that way, which is by definition clock
gating.  Thus, "gated-clock" and "Simple gated clock generator"
would fit very well.

> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +properties:
> +  compatible:
> +    const: voltage-oscillator
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  clock-frequency: true
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  enable-gpios:
> +    description:
> +      Contains a single GPIO specifier for the GPIO that enables and 
> disables
> +      the oscillator.
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: handle of the regulator that provides the supply 
> voltage
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clock-frequency
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    voltage-oscillator {
> +      compatible = "voltage-oscillator";
> +      #clock-cells = <0>;
> +      clock-frequency = <1000000000>;
> +      vdd-supply = <&reg_vdd>;
> +    };
> +...

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Hi Dragan,

Am Montag, 15. Juli 2024, 17:15:45 CEST schrieb Dragan Simic:
> On 2024-07-15 13:02, Heiko Stuebner wrote:
> > In contrast to fixed clocks that are described as ungateable, boards
> > sometimes use additional oscillators for things like PCIe reference
> > clocks, that need actual supplies to get enabled and enable-gpios to be
> > toggled for them to work.
> > 
> > This adds a binding for such oscillators that are not configurable
> > themself, but need to handle supplies for them to work.
> > 
> > In schematics they often can be seen as
> > 
> >          ----------------
> > Enable - | 100MHz,3.3V, | - VDD
> >          |    3225      |
> >    GND - |              | - OUT
> >          ----------------
> > 
> > or similar. The enable pin might be separate but can also just be tied
> > to the vdd supply, hence it is optional in the binding.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > new file mode 100644
> > index 0000000000000..8bff6b0fd582e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Voltage controlled oscillator
> 
> Frankly, I find the "voltage-oscillator" and "voltage controlled
> oscillator" names awkward.  In general, "clock" is used throughout
> the entire kernel, when it comes to naming files and defining
> "compatible" strings.  Thus, I'd suggest that "clock" is used here
> instead of "oscillator", because it's consistent and shorter.
>
> How about using "gated-clock" for the "compatible" string, and
> "Simple gated clock generator" instead of "voltage controlled
> oscillator"?  Besides sounding awkward, "voltage controlled
> oscillator" may suggest that the clock generator can be adjusted
> or programmed somehow by applying the voltage, while it can only
> be enabled or disabled that way, which is by definition clock
> gating.  Thus, "gated-clock" and "Simple gated clock generator"
> would fit very well.

The naming came from Stephen - one of the clock maintainers ;-)
See discussion in v1. Who also described these things as
"voltage-controlled-oscillators".

And from that discussion I also got the impression we should aim for
more specific naming - especially when talking about dt-bindings, for this
"usage in the Linux kernel" actually isn't a suitable metric and
"gated-clock" is probably way too generic I think.

Though I'm not attached to any specific naming, so we'll simply
wait for the clock- and dt-maintainers to weigh in ;-)


Heiko

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

On 2024-07-15 19:46, Heiko Stübner wrote:
> Am Montag, 15. Juli 2024, 17:15:45 CEST schrieb Dragan Simic:
>> On 2024-07-15 13:02, Heiko Stuebner wrote:
>> > In contrast to fixed clocks that are described as ungateable, boards
>> > sometimes use additional oscillators for things like PCIe reference
>> > clocks, that need actual supplies to get enabled and enable-gpios to be
>> > toggled for them to work.
>> >
>> > This adds a binding for such oscillators that are not configurable
>> > themself, but need to handle supplies for them to work.
>> >
>> > In schematics they often can be seen as
>> >
>> >          ----------------
>> > Enable - | 100MHz,3.3V, | - VDD
>> >          |    3225      |
>> >    GND - |              | - OUT
>> >          ----------------
>> >
>> > or similar. The enable pin might be separate but can also just be tied
>> > to the vdd supply, hence it is optional in the binding.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>> >  1 file changed, 49 insertions(+)
>> >  create mode 100644
>> > Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >
>> > diff --git
>> > a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> > b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> > new file mode 100644
>> > index 0000000000000..8bff6b0fd582e
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> > @@ -0,0 +1,49 @@
>> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Voltage controlled oscillator
>> 
>> Frankly, I find the "voltage-oscillator" and "voltage controlled
>> oscillator" names awkward.  In general, "clock" is used throughout
>> the entire kernel, when it comes to naming files and defining
>> "compatible" strings.  Thus, I'd suggest that "clock" is used here
>> instead of "oscillator", because it's consistent and shorter.
>> 
>> How about using "gated-clock" for the "compatible" string, and
>> "Simple gated clock generator" instead of "voltage controlled
>> oscillator"?  Besides sounding awkward, "voltage controlled
>> oscillator" may suggest that the clock generator can be adjusted
>> or programmed somehow by applying the voltage, while it can only
>> be enabled or disabled that way, which is by definition clock
>> gating.  Thus, "gated-clock" and "Simple gated clock generator"
>> would fit very well.
> 
> The naming came from Stephen - one of the clock maintainers ;-)
> See discussion in v1. Who also described these things as
> "voltage-controlled-oscillators".
> 
> And from that discussion I also got the impression we should aim for
> more specific naming - especially when talking about dt-bindings, for 
> this
> "usage in the Linux kernel" actually isn't a suitable metric and
> "gated-clock" is probably way too generic I think.

I see, thanks for the clarification.  Though, the generic nature of
"gated-clock" as the name may actually make this driver a bit more
future-proof, by allowing some other features to be added to it at
some point in the future, avoiding that way the need for yet another
kernel driver.

> Though I'm not attached to any specific naming, so we'll simply
> wait for the clock- and dt-maintainers to weigh in ;-)

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Am Montag, 15. Juli 2024, 20:01:35 CEST schrieb Dragan Simic:
> On 2024-07-15 19:46, Heiko Stübner wrote:
> > Am Montag, 15. Juli 2024, 17:15:45 CEST schrieb Dragan Simic:
> >> On 2024-07-15 13:02, Heiko Stuebner wrote:
> >> > In contrast to fixed clocks that are described as ungateable, boards
> >> > sometimes use additional oscillators for things like PCIe reference
> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
> >> > toggled for them to work.
> >> >
> >> > This adds a binding for such oscillators that are not configurable
> >> > themself, but need to handle supplies for them to work.
> >> >
> >> > In schematics they often can be seen as
> >> >
> >> >          ----------------
> >> > Enable - | 100MHz,3.3V, | - VDD
> >> >          |    3225      |
> >> >    GND - |              | - OUT
> >> >          ----------------
> >> >
> >> > or similar. The enable pin might be separate but can also just be tied
> >> > to the vdd supply, hence it is optional in the binding.
> >> >
> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> >> > ---
> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >> >  1 file changed, 49 insertions(+)
> >> >  create mode 100644
> >> > Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >
> >> > diff --git
> >> > a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > new file mode 100644
> >> > index 0000000000000..8bff6b0fd582e
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > @@ -0,0 +1,49 @@
> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> > +%YAML 1.2
> >> > +---
> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> > +
> >> > +title: Voltage controlled oscillator
> >> 
> >> Frankly, I find the "voltage-oscillator" and "voltage controlled
> >> oscillator" names awkward.  In general, "clock" is used throughout
> >> the entire kernel, when it comes to naming files and defining
> >> "compatible" strings.  Thus, I'd suggest that "clock" is used here
> >> instead of "oscillator", because it's consistent and shorter.
> >> 
> >> How about using "gated-clock" for the "compatible" string, and
> >> "Simple gated clock generator" instead of "voltage controlled
> >> oscillator"?  Besides sounding awkward, "voltage controlled
> >> oscillator" may suggest that the clock generator can be adjusted
> >> or programmed somehow by applying the voltage, while it can only
> >> be enabled or disabled that way, which is by definition clock
> >> gating.  Thus, "gated-clock" and "Simple gated clock generator"
> >> would fit very well.
> > 
> > The naming came from Stephen - one of the clock maintainers ;-)
> > See discussion in v1. Who also described these things as
> > "voltage-controlled-oscillators".
> > 
> > And from that discussion I also got the impression we should aim for
> > more specific naming - especially when talking about dt-bindings, for 
> > this
> > "usage in the Linux kernel" actually isn't a suitable metric and
> > "gated-clock" is probably way too generic I think.
> 
> I see, thanks for the clarification.  Though, the generic nature of
> "gated-clock" as the name may actually make this driver a bit more
> future-proof, by allowing some other features to be added to it at
> some point in the future, avoiding that way the need for yet another
> kernel driver.

you're talking about the driver ... we're in the hardware-binding here.
Those are two completely different topics ;-) .

Devicetree is always about describing the hardware as best as possible,
so you don't want too many "generics" there, because we're always talking
about specific ICs soldered to some board.

I also "violated" that in my v1 by grouping in the the Diodes parts, which
as Stephen pointed out are quite different afterall.


Heiko

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> In contrast to fixed clocks that are described as ungateable, boards
> sometimes use additional oscillators for things like PCIe reference
> clocks, that need actual supplies to get enabled and enable-gpios to be
> toggled for them to work.
> 
> This adds a binding for such oscillators that are not configurable
> themself, but need to handle supplies for them to work.
> 
> In schematics they often can be seen as
> 
>          ----------------
> Enable - | 100MHz,3.3V, | - VDD
>          |    3225      |
>    GND - |              | - OUT
>          ----------------
> 
> or similar. The enable pin might be separate but can also just be tied
> to the vdd supply, hence it is optional in the binding.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> new file mode 100644
> index 0000000000000..8bff6b0fd582e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Voltage controlled oscillator

Voltage controlled oscillator? Really? That sounds far too similar to a
VCO to me, and the input voltage here (according to the description at
least) does not affect the frequency of oscillation.

Why the dedicated binding, rather than adding a supply and enable-gpio
to the existing "fixed-clock" binding? I suspect that a large portion of
"fixed-clock"s actually require a supply that is (effectively)
always-on.

Cheers,
Conor.

> +
> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +properties:
> +  compatible:
> +    const: voltage-oscillator
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  clock-frequency: true
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  enable-gpios:
> +    description:
> +      Contains a single GPIO specifier for the GPIO that enables and disables
> +      the oscillator.
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: handle of the regulator that provides the supply voltage
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clock-frequency
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    voltage-oscillator {
> +      compatible = "voltage-oscillator";
> +      #clock-cells = <0>;
> +      clock-frequency = <1000000000>;
> +      vdd-supply = <&reg_vdd>;
> +    };
> +...
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Conor,

On 2024-07-16 18:15, Conor Dooley wrote:
> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
>> In contrast to fixed clocks that are described as ungateable, boards
>> sometimes use additional oscillators for things like PCIe reference
>> clocks, that need actual supplies to get enabled and enable-gpios to 
>> be
>> toggled for them to work.
>> 
>> This adds a binding for such oscillators that are not configurable
>> themself, but need to handle supplies for them to work.
>> 
>> In schematics they often can be seen as
>> 
>>          ----------------
>> Enable - | 100MHz,3.3V, | - VDD
>>          |    3225      |
>>    GND - |              | - OUT
>>          ----------------
>> 
>> or similar. The enable pin might be separate but can also just be tied
>> to the vdd supply, hence it is optional in the binding.
>> 
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>  .../bindings/clock/voltage-oscillator.yaml    | 49 
>> +++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml 
>> b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> new file mode 100644
>> index 0000000000000..8bff6b0fd582e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> @@ -0,0 +1,49 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Voltage controlled oscillator
> 
> Voltage controlled oscillator? Really? That sounds far too similar to a
> VCO to me, and the input voltage here (according to the description at
> least) does not affect the frequency of oscillation.

Yup, "voltage controlled oscillator" really isn't the right choice for
the name, as I wrote about already. [1]

[1] 
https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/

> Why the dedicated binding, rather than adding a supply and enable-gpio
> to the existing "fixed-clock" binding? I suspect that a large portion 
> of
> "fixed-clock"s actually require a supply that is (effectively)
> always-on.
> 
>> +
>> +maintainers:
>> +  - Heiko Stuebner <heiko@sntech.de>
>> +
>> +properties:
>> +  compatible:
>> +    const: voltage-oscillator
>> +
>> +  "#clock-cells":
>> +    const: 0
>> +
>> +  clock-frequency: true
>> +
>> +  clock-output-names:
>> +    maxItems: 1
>> +
>> +  enable-gpios:
>> +    description:
>> +      Contains a single GPIO specifier for the GPIO that enables and 
>> disables
>> +      the oscillator.
>> +    maxItems: 1
>> +
>> +  vdd-supply:
>> +    description: handle of the regulator that provides the supply 
>> voltage
>> +
>> +required:
>> +  - compatible
>> +  - "#clock-cells"
>> +  - clock-frequency
>> +  - vdd-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    voltage-oscillator {
>> +      compatible = "voltage-oscillator";
>> +      #clock-cells = <0>;
>> +      clock-frequency = <1000000000>;
>> +      vdd-supply = <&reg_vdd>;
>> +    };
>> +...
>> --
>> 2.39.2
>> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Heiko,

On 2024-07-15 21:13, Heiko Stübner wrote:
> Am Montag, 15. Juli 2024, 20:01:35 CEST schrieb Dragan Simic:
>> On 2024-07-15 19:46, Heiko Stübner wrote:
>> > Am Montag, 15. Juli 2024, 17:15:45 CEST schrieb Dragan Simic:
>> >> On 2024-07-15 13:02, Heiko Stuebner wrote:
>> >> > In contrast to fixed clocks that are described as ungateable, boards
>> >> > sometimes use additional oscillators for things like PCIe reference
>> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
>> >> > toggled for them to work.
>> >> >
>> >> > This adds a binding for such oscillators that are not configurable
>> >> > themself, but need to handle supplies for them to work.
>> >> >
>> >> > In schematics they often can be seen as
>> >> >
>> >> >          ----------------
>> >> > Enable - | 100MHz,3.3V, | - VDD
>> >> >          |    3225      |
>> >> >    GND - |              | - OUT
>> >> >          ----------------
>> >> >
>> >> > or similar. The enable pin might be separate but can also just be tied
>> >> > to the vdd supply, hence it is optional in the binding.
>> >> >
>> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> >> > ---
>> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>> >> >  1 file changed, 49 insertions(+)
>> >> >  create mode 100644
>> >> > Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> >
>> >> > diff --git
>> >> > a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> > b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> > new file mode 100644
>> >> > index 0000000000000..8bff6b0fd582e
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> > @@ -0,0 +1,49 @@
>> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> > +%YAML 1.2
>> >> > +---
>> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> > +
>> >> > +title: Voltage controlled oscillator
>> >>
>> >> Frankly, I find the "voltage-oscillator" and "voltage controlled
>> >> oscillator" names awkward.  In general, "clock" is used throughout
>> >> the entire kernel, when it comes to naming files and defining
>> >> "compatible" strings.  Thus, I'd suggest that "clock" is used here
>> >> instead of "oscillator", because it's consistent and shorter.
>> >>
>> >> How about using "gated-clock" for the "compatible" string, and
>> >> "Simple gated clock generator" instead of "voltage controlled
>> >> oscillator"?  Besides sounding awkward, "voltage controlled
>> >> oscillator" may suggest that the clock generator can be adjusted
>> >> or programmed somehow by applying the voltage, while it can only
>> >> be enabled or disabled that way, which is by definition clock
>> >> gating.  Thus, "gated-clock" and "Simple gated clock generator"
>> >> would fit very well.
>> >
>> > The naming came from Stephen - one of the clock maintainers ;-)
>> > See discussion in v1. Who also described these things as
>> > "voltage-controlled-oscillators".
>> >
>> > And from that discussion I also got the impression we should aim for
>> > more specific naming - especially when talking about dt-bindings, for
>> > this
>> > "usage in the Linux kernel" actually isn't a suitable metric and
>> > "gated-clock" is probably way too generic I think.
>> 
>> I see, thanks for the clarification.  Though, the generic nature of
>> "gated-clock" as the name may actually make this driver a bit more
>> future-proof, by allowing some other features to be added to it at
>> some point in the future, avoiding that way the need for yet another
>> kernel driver.
> 
> you're talking about the driver ... we're in the hardware-binding here.
> Those are two completely different topics ;-) .
> 
> Devicetree is always about describing the hardware as best as possible,
> so you don't want too many "generics" there, because we're always 
> talking
> about specific ICs soldered to some board.
> 
> I also "violated" that in my v1 by grouping in the the Diodes parts, 
> which
> as Stephen pointed out are quite different afterall.

I'll make sure to go through the v1 discussion in detail ASAP.  After
that, I'll come back with some more thoughts.

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Anand Moon]

Hi Heiko,

On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
>
> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and its
> SATA controller with 2 lanes each. The supply for the refclk oscillator is
> the same that supplies the M.2 slot, but the SATA controller port is
> supplied by a different rail.
>
> This leads to the effect that if the PCIe30x4 controller for the M.2
> probes first, everything works normally. But if the PCIe30x2 controller
> that is connected to the SATA controller probes first, it will hang on
> the first DBI read as nothing will have enabled the refclock before.
>

I just checked the rk3588-rock-5-itx.dts in the linux-next.
You have not enabled sata0 and sata2, which might be the problem
for the SATA controller not getting initialized.

Thanks
-Anand

> Fix this by describing the clock generator with its supplies so that
> both controllers can reference it as needed.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../boot/dts/rockchip/rk3588-rock-5-itx.dts   | 38 ++++++++++++++++++-
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
> index d0b922b8d67e8..37bc53f2796fc 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
> @@ -72,6 +72,15 @@ hdd-led2 {
>                 };
>         };
>
> +       /* Unnamed voltage oscillator: 100MHz,3.3V,3225 */
> +       pcie30_port0_refclk: pcie30_port1_refclk: pcie-voltage-oscillator {
> +               compatible = "voltage-oscillator";
> +               #clock-cells = <0>;
> +               clock-frequency = <100000000>;
> +               clock-output-names = "pcie30_refclk";
> +               vdd-supply = <&vcc3v3_pi6c_05>;
> +       };
> +
>         fan0: pwm-fan {
>                 compatible = "pwm-fan";
>                 #cooling-cells = <2>;
> @@ -146,13 +155,14 @@ vcc3v3_lan: vcc3v3_lan_phy2: regulator-vcc3v3-lan {
>                 vin-supply = <&vcc_3v3_s3>;
>         };
>
> -       vcc3v3_mkey: regulator-vcc3v3-mkey {
> +       /* The PCIE30x4_PWREN_H controls two regulators */
> +       vcc3v3_mkey: vcc3v3_pi6c_05: regulator-vcc3v3-pi6c-05 {
>                 compatible = "regulator-fixed";
>                 enable-active-high;
>                 gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_HIGH>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pcie30x4_pwren_h>;
> -               regulator-name = "vcc3v3_mkey";
> +               regulator-name = "vcc3v3_pi6c_05";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 startup-delay-us = <5000>;
> @@ -513,6 +523,18 @@ &pcie30phy {
>
>  /* ASMedia ASM1164 Sata controller */
>  &pcie3x2 {
> +       /*
> +        * The board has a "pcie_refclk" oscillator that needs enabling,
> +        * so add it to the list of clocks.
> +        */
> +       clocks = <&cru ACLK_PCIE_2L_MSTR>, <&cru ACLK_PCIE_2L_SLV>,
> +                <&cru ACLK_PCIE_2L_DBI>, <&cru PCLK_PCIE_2L>,
> +                <&cru CLK_PCIE_AUX1>, <&cru CLK_PCIE2L_PIPE>,
> +                <&pcie30_port1_refclk>;
> +       clock-names = "aclk_mst", "aclk_slv",
> +                     "aclk_dbi", "pclk",
> +                     "aux", "pipe",
> +                     "ref";
>         pinctrl-names = "default";
>         pinctrl-0 = <&pcie30x2_perstn_m1_l>;
>         reset-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> @@ -522,6 +544,18 @@ &pcie3x2 {
>
>  /* M.2 M.key */
>  &pcie3x4 {
> +       /*
> +        * The board has a "pcie_refclk" oscillator that needs enabling,
> +        * so add it to the list of clocks.
> +        */
> +       clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
> +                <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
> +                <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>,
> +                <&pcie30_port0_refclk>;
> +       clock-names = "aclk_mst", "aclk_slv",
> +                     "aclk_dbi", "pclk",
> +                     "aux", "pipe",
> +                     "ref";
>         num-lanes = <2>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pcie30x4_perstn_m1_l>;
> --
> 2.39.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Dragan Simic]

Hello Anand,

On 2024-07-18 09:26, Anand Moon wrote:
> On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
>> 
>> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and 
>> its
>> SATA controller with 2 lanes each. The supply for the refclk 
>> oscillator is
>> the same that supplies the M.2 slot, but the SATA controller port is
>> supplied by a different rail.
>> 
>> This leads to the effect that if the PCIe30x4 controller for the M.2
>> probes first, everything works normally. But if the PCIe30x2 
>> controller
>> that is connected to the SATA controller probes first, it will hang on
>> the first DBI read as nothing will have enabled the refclock before.
> 
> I just checked the rk3588-rock-5-itx.dts in the linux-next.
> You have not enabled sata0 and sata2, which might be the problem
> for the SATA controller not getting initialized.

Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's 
fine.
Please have a look at the board schematic, it uses a separate PCI 
Express
SATA controller for its four SATA ports.

>> Fix this by describing the clock generator with its supplies so that
>> both controllers can reference it as needed.
>> 
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>  .../boot/dts/rockchip/rk3588-rock-5-itx.dts   | 38 
>> ++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> index d0b922b8d67e8..37bc53f2796fc 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> @@ -72,6 +72,15 @@ hdd-led2 {
>>                 };
>>         };
>> 
>> +       /* Unnamed voltage oscillator: 100MHz,3.3V,3225 */
>> +       pcie30_port0_refclk: pcie30_port1_refclk: 
>> pcie-voltage-oscillator {
>> +               compatible = "voltage-oscillator";
>> +               #clock-cells = <0>;
>> +               clock-frequency = <100000000>;
>> +               clock-output-names = "pcie30_refclk";
>> +               vdd-supply = <&vcc3v3_pi6c_05>;
>> +       };
>> +
>>         fan0: pwm-fan {
>>                 compatible = "pwm-fan";
>>                 #cooling-cells = <2>;
>> @@ -146,13 +155,14 @@ vcc3v3_lan: vcc3v3_lan_phy2: 
>> regulator-vcc3v3-lan {
>>                 vin-supply = <&vcc_3v3_s3>;
>>         };
>> 
>> -       vcc3v3_mkey: regulator-vcc3v3-mkey {
>> +       /* The PCIE30x4_PWREN_H controls two regulators */
>> +       vcc3v3_mkey: vcc3v3_pi6c_05: regulator-vcc3v3-pi6c-05 {
>>                 compatible = "regulator-fixed";
>>                 enable-active-high;
>>                 gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_HIGH>;
>>                 pinctrl-names = "default";
>>                 pinctrl-0 = <&pcie30x4_pwren_h>;
>> -               regulator-name = "vcc3v3_mkey";
>> +               regulator-name = "vcc3v3_pi6c_05";
>>                 regulator-min-microvolt = <3300000>;
>>                 regulator-max-microvolt = <3300000>;
>>                 startup-delay-us = <5000>;
>> @@ -513,6 +523,18 @@ &pcie30phy {
>> 
>>  /* ASMedia ASM1164 Sata controller */
>>  &pcie3x2 {
>> +       /*
>> +        * The board has a "pcie_refclk" oscillator that needs 
>> enabling,
>> +        * so add it to the list of clocks.
>> +        */
>> +       clocks = <&cru ACLK_PCIE_2L_MSTR>, <&cru ACLK_PCIE_2L_SLV>,
>> +                <&cru ACLK_PCIE_2L_DBI>, <&cru PCLK_PCIE_2L>,
>> +                <&cru CLK_PCIE_AUX1>, <&cru CLK_PCIE2L_PIPE>,
>> +                <&pcie30_port1_refclk>;
>> +       clock-names = "aclk_mst", "aclk_slv",
>> +                     "aclk_dbi", "pclk",
>> +                     "aux", "pipe",
>> +                     "ref";
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pcie30x2_perstn_m1_l>;
>>         reset-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
>> @@ -522,6 +544,18 @@ &pcie3x2 {
>> 
>>  /* M.2 M.key */
>>  &pcie3x4 {
>> +       /*
>> +        * The board has a "pcie_refclk" oscillator that needs 
>> enabling,
>> +        * so add it to the list of clocks.
>> +        */
>> +       clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
>> +                <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
>> +                <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>,
>> +                <&pcie30_port0_refclk>;
>> +       clock-names = "aclk_mst", "aclk_slv",
>> +                     "aclk_dbi", "pclk",
>> +                     "aux", "pipe",
>> +                     "ref";
>>         num-lanes = <2>;
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pcie30x4_perstn_m1_l>;
>> --
>> 2.39.2

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Anand Moon]

Hi Dragan,

On Thu, 18 Jul 2024 at 13:02, Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Anand,
>
> On 2024-07-18 09:26, Anand Moon wrote:
> > On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
> >>
> >> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and
> >> its
> >> SATA controller with 2 lanes each. The supply for the refclk
> >> oscillator is
> >> the same that supplies the M.2 slot, but the SATA controller port is
> >> supplied by a different rail.
> >>
> >> This leads to the effect that if the PCIe30x4 controller for the M.2
> >> probes first, everything works normally. But if the PCIe30x2
> >> controller
> >> that is connected to the SATA controller probes first, it will hang on
> >> the first DBI read as nothing will have enabled the refclock before.
> >
> > I just checked the rk3588-rock-5-itx.dts in the linux-next.
> > You have not enabled sata0 and sata2, which might be the problem
> > for the SATA controller not getting initialized.
>
> Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's
> fine.
> Please have a look at the board schematic, it uses a separate PCI
> Express
> SATA controller for its four SATA ports.
>
yes, But I am referring to sata node not enabled which enable the PHY_TYPE_SATA.

see rk3588-coolpi-cm5-evb.dts and rk3588-edgeble-neu6a-io.dtsi
rk3588-quartzpro64.dts
which have sata port on board.

&sata0 {
        status = "okay";
};

Thanks
-Anand

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Dragan Simic]

On 2024-07-18 09:52, Anand Moon wrote:
> On Thu, 18 Jul 2024 at 13:02, Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-07-18 09:26, Anand Moon wrote:
>> > On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
>> >>
>> >> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and
>> >> its
>> >> SATA controller with 2 lanes each. The supply for the refclk
>> >> oscillator is
>> >> the same that supplies the M.2 slot, but the SATA controller port is
>> >> supplied by a different rail.
>> >>
>> >> This leads to the effect that if the PCIe30x4 controller for the M.2
>> >> probes first, everything works normally. But if the PCIe30x2
>> >> controller
>> >> that is connected to the SATA controller probes first, it will hang on
>> >> the first DBI read as nothing will have enabled the refclock before.
>> >
>> > I just checked the rk3588-rock-5-itx.dts in the linux-next.
>> > You have not enabled sata0 and sata2, which might be the problem
>> > for the SATA controller not getting initialized.
>> 
>> Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's
>> fine.
>> Please have a look at the board schematic, it uses a separate PCI
>> Express
>> SATA controller for its four SATA ports.
>> 
> yes, But I am referring to sata node not enabled which enable
> the PHY_TYPE_SATA.
> 
> see rk3588-coolpi-cm5-evb.dts and rk3588-edgeble-neu6a-io.dtsi
> rk3588-quartzpro64.dts
> which have sata port on board.
> 
> &sata0 {
>         status = "okay";
> };

QuartzPro64, as an example, uses RK3588's built-in SATA interfaces,
so it enables sata0 in its board dts.  Rock 5 ITX doesn't do that,
as I already described.

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Anand Moon]

Hi Dragan

On Thu, 18 Jul 2024 at 13:28, Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-07-18 09:52, Anand Moon wrote:
> > On Thu, 18 Jul 2024 at 13:02, Dragan Simic <dsimic@manjaro.org> wrote:
> >> On 2024-07-18 09:26, Anand Moon wrote:
> >> > On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
> >> >>
> >> >> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and
> >> >> its
> >> >> SATA controller with 2 lanes each. The supply for the refclk
> >> >> oscillator is
> >> >> the same that supplies the M.2 slot, but the SATA controller port is
> >> >> supplied by a different rail.
> >> >>
> >> >> This leads to the effect that if the PCIe30x4 controller for the M.2
> >> >> probes first, everything works normally. But if the PCIe30x2
> >> >> controller
> >> >> that is connected to the SATA controller probes first, it will hang on
> >> >> the first DBI read as nothing will have enabled the refclock before.
> >> >
> >> > I just checked the rk3588-rock-5-itx.dts in the linux-next.
> >> > You have not enabled sata0 and sata2, which might be the problem
> >> > for the SATA controller not getting initialized.
> >>
> >> Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's
> >> fine.
> >> Please have a look at the board schematic, it uses a separate PCI
> >> Express
> >> SATA controller for its four SATA ports.
> >>
> > yes, But I am referring to sata node not enabled which enable
> > the PHY_TYPE_SATA.
> >
> > see rk3588-coolpi-cm5-evb.dts and rk3588-edgeble-neu6a-io.dtsi
> > rk3588-quartzpro64.dts
> > which have sata port on board.
> >
> > &sata0 {
> >         status = "okay";
> > };
>
> QuartzPro64, as an example, uses RK3588's built-in SATA interfaces,
> so it enables sata0 in its board dts.  Rock 5 ITX doesn't do that,
> as I already described.

Ok no problem,

Thanks
-Anand

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Hi Conor,

Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> > In contrast to fixed clocks that are described as ungateable, boards
> > sometimes use additional oscillators for things like PCIe reference
> > clocks, that need actual supplies to get enabled and enable-gpios to be
> > toggled for them to work.
> > 
> > This adds a binding for such oscillators that are not configurable
> > themself, but need to handle supplies for them to work.
> > 
> > In schematics they often can be seen as
> > 
> >          ----------------
> > Enable - | 100MHz,3.3V, | - VDD
> >          |    3225      |
> >    GND - |              | - OUT
> >          ----------------
> > 
> > or similar. The enable pin might be separate but can also just be tied
> > to the vdd supply, hence it is optional in the binding.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > new file mode 100644
> > index 0000000000000..8bff6b0fd582e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Voltage controlled oscillator
> 
> Voltage controlled oscillator? Really? That sounds far too similar to a
> VCO to me, and the input voltage here (according to the description at
> least) does not affect the frequency of oscillation.

That naming was suggested by Stephen in v1 [0] .

Of course the schematics for the board I have only describe it as
"100MHz,3.3V,3225" , thumbing through some mouser parts matching that
only mentions "supply voltage" in their datasheets but not a dependency
between rate and voltage.

[0] https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/

> Why the dedicated binding, rather than adding a supply and enable-gpio
> to the existing "fixed-clock" binding? I suspect that a large portion of
> "fixed-clock"s actually require a supply that is (effectively)
> always-on.

I guess there are three aspects:
- I do remember discussions in the past about not extending generic
  bindings with device-specific stuff. I think generic power-sequences
  were the topic back then, though that might have changed over time?
- There are places that describe "fixed-clock" as
  "basic fixed-rate clock that cannot gate" [1]
- Stephen also suggested a separate binding [2]

With the fixed-clock being sort of the root for everything else on most
systems, I opted to leave it alone. I guess if the consenus really is that
this should go there, I can move it, but discussion in v1 

Interestingly the fixed clock had a gpios property 10 years ago [3] :-) .

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
[2] https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
[3] https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t


Heiko


> > +
> > +maintainers:
> > +  - Heiko Stuebner <heiko@sntech.de>
> > +
> > +properties:
> > +  compatible:
> > +    const: voltage-oscillator
> > +
> > +  "#clock-cells":
> > +    const: 0
> > +
> > +  clock-frequency: true
> > +
> > +  clock-output-names:
> > +    maxItems: 1
> > +
> > +  enable-gpios:
> > +    description:
> > +      Contains a single GPIO specifier for the GPIO that enables and disables
> > +      the oscillator.
> > +    maxItems: 1
> > +
> > +  vdd-supply:
> > +    description: handle of the regulator that provides the supply voltage
> > +
> > +required:
> > +  - compatible
> > +  - "#clock-cells"
> > +  - clock-frequency
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    voltage-oscillator {
> > +      compatible = "voltage-oscillator";
> > +      #clock-cells = <0>;
> > +      clock-frequency = <1000000000>;
> > +      vdd-supply = <&reg_vdd>;
> > +    };
> > +...
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Heiko Stübner]

Am Donnerstag, 18. Juli 2024, 10:00:51 CEST schrieb Anand Moon:
> Hi Dragan
> 
> On Thu, 18 Jul 2024 at 13:28, Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > On 2024-07-18 09:52, Anand Moon wrote:
> > > On Thu, 18 Jul 2024 at 13:02, Dragan Simic <dsimic@manjaro.org> wrote:
> > >> On 2024-07-18 09:26, Anand Moon wrote:
> > >> > On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
> > >> >>
> > >> >> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and
> > >> >> its
> > >> >> SATA controller with 2 lanes each. The supply for the refclk
> > >> >> oscillator is
> > >> >> the same that supplies the M.2 slot, but the SATA controller port is
> > >> >> supplied by a different rail.
> > >> >>
> > >> >> This leads to the effect that if the PCIe30x4 controller for the M.2
> > >> >> probes first, everything works normally. But if the PCIe30x2
> > >> >> controller
> > >> >> that is connected to the SATA controller probes first, it will hang on
> > >> >> the first DBI read as nothing will have enabled the refclock before.
> > >> >
> > >> > I just checked the rk3588-rock-5-itx.dts in the linux-next.
> > >> > You have not enabled sata0 and sata2, which might be the problem
> > >> > for the SATA controller not getting initialized.
> > >>
> > >> Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's
> > >> fine.
> > >> Please have a look at the board schematic, it uses a separate PCI
> > >> Express
> > >> SATA controller for its four SATA ports.
> > >>
> > > yes, But I am referring to sata node not enabled which enable
> > > the PHY_TYPE_SATA.
> > >
> > > see rk3588-coolpi-cm5-evb.dts and rk3588-edgeble-neu6a-io.dtsi
> > > rk3588-quartzpro64.dts
> > > which have sata port on board.
> > >
> > > &sata0 {
> > >         status = "okay";
> > > };
> >
> > QuartzPro64, as an example, uses RK3588's built-in SATA interfaces,
> > so it enables sata0 in its board dts.  Rock 5 ITX doesn't do that,
> > as I already described.
> 
> Ok no problem,

For the Rock 5 ITX it really only routes 2 PCIe lanes to one M.2 port
and the other 2 lanes to the separate ASMedia SATA controller.
So from the Rock5 PoV, it's really just 2 PCIe 2-lane slots and the
SATA controller simply gets probed as PCIe device.

I even have a sample of the Rock 5+ here, that actually drops the
separate SATA controller and instead provides a 2nd M.2 slot ;-)

Heiko

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello all,

On 2024-07-18 11:25, Heiko Stübner wrote:
> Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
>> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
>> > In contrast to fixed clocks that are described as ungateable, boards
>> > sometimes use additional oscillators for things like PCIe reference
>> > clocks, that need actual supplies to get enabled and enable-gpios to be
>> > toggled for them to work.
>> >
>> > This adds a binding for such oscillators that are not configurable
>> > themself, but need to handle supplies for them to work.
>> >
>> > In schematics they often can be seen as
>> >
>> >          ----------------
>> > Enable - | 100MHz,3.3V, | - VDD
>> >          |    3225      |
>> >    GND - |              | - OUT
>> >          ----------------
>> >
>> > or similar. The enable pin might be separate but can also just be tied
>> > to the vdd supply, hence it is optional in the binding.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>> >  1 file changed, 49 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >
>> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> > new file mode 100644
>> > index 0000000000000..8bff6b0fd582e
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> > @@ -0,0 +1,49 @@
>> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Voltage controlled oscillator
>> 
>> Voltage controlled oscillator? Really? That sounds far too similar to 
>> a
>> VCO to me, and the input voltage here (according to the description at
>> least) does not affect the frequency of oscillation.
> 
> That naming was suggested by Stephen in v1 [0] .
> 
> Of course the schematics for the board I have only describe it as
> "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
> only mentions "supply voltage" in their datasheets but not a dependency
> between rate and voltage.
> 
> [0] 
> https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> 
>> Why the dedicated binding, rather than adding a supply and enable-gpio
>> to the existing "fixed-clock" binding? I suspect that a large portion 
>> of
>> "fixed-clock"s actually require a supply that is (effectively)
>> always-on.
> 
> I guess there are three aspects:
> - I do remember discussions in the past about not extending generic
>   bindings with device-specific stuff. I think generic power-sequences
>   were the topic back then, though that might have changed over time?
> - There are places that describe "fixed-clock" as
>   "basic fixed-rate clock that cannot gate" [1]
> - Stephen also suggested a separate binding [2]
> 
> With the fixed-clock being sort of the root for everything else on most
> systems, I opted to leave it alone. I guess if the consenus really is 
> that
> this should go there, I can move it, but discussion in v1
> 
> Interestingly the fixed clock had a gpios property 10 years ago [3] :-) 
> .
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
> [2] 
> https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
> [3] 
> https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t

After finally going through the v1 discussion [4] in detail,
here are my further thoughts:

- I agree with dropping the Diodes stuff, [5] because I see
   no need for that at this point;  though, am I missing
   something, where are they actually used?

- I agree that "enable-gpios" and "vdd-supply" should be
   required in the binding, [5] because that's the basis for
   something to be actually represented this way

- I agree that it should be better not to touch fixed-clock
   at this point, simply because it's used in very many places,
   and because in this case we need more than it requires (see
   the bullet point above)

- As I wrote already, [6] I highly disagree with this being
   called voltage-controlled oscillator (VCO), [7] simply
   because it isn't a VCO, but a clock that can be gated;
   though, looking forward to what the last bullet point
   asks to be clarified further

- I still think that gated-clock is the best choice for the
   name, because it uses "clock" that's used throughout the
   entire codebase, and uses "gated" to reflect the nature
   of the clock generator

- Maybe we could actually use fixed-gated-clock as the name,
   which would make more sense from the stanpoint of possibly
   merging it into fixed-clock at some point, but I'd like
   to hear first what's actually going on with the Diodes
   stuff that was deleted in v2, which I already asked about
   in the first bullet point

[4] 
https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
[5] 
https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
[6] 
https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
[7] 
https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/

>> > +
>> > +maintainers:
>> > +  - Heiko Stuebner <heiko@sntech.de>
>> > +
>> > +properties:
>> > +  compatible:
>> > +    const: voltage-oscillator
>> > +
>> > +  "#clock-cells":
>> > +    const: 0
>> > +
>> > +  clock-frequency: true
>> > +
>> > +  clock-output-names:
>> > +    maxItems: 1
>> > +
>> > +  enable-gpios:
>> > +    description:
>> > +      Contains a single GPIO specifier for the GPIO that enables and disables
>> > +      the oscillator.
>> > +    maxItems: 1
>> > +
>> > +  vdd-supply:
>> > +    description: handle of the regulator that provides the supply voltage
>> > +
>> > +required:
>> > +  - compatible
>> > +  - "#clock-cells"
>> > +  - clock-frequency
>> > +  - vdd-supply
>> > +
>> > +additionalProperties: false
>> > +
>> > +examples:
>> > +  - |
>> > +    voltage-oscillator {
>> > +      compatible = "voltage-oscillator";
>> > +      #clock-cells = <0>;
>> > +      clock-frequency = <1000000000>;
>> > +      vdd-supply = <&reg_vdd>;
>> > +    };
>> > +...

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Hi Dragan,

Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:
> On 2024-07-18 11:25, Heiko Stübner wrote:
> > Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
> >> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> >> > In contrast to fixed clocks that are described as ungateable, boards
> >> > sometimes use additional oscillators for things like PCIe reference
> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
> >> > toggled for them to work.
> >> >
> >> > This adds a binding for such oscillators that are not configurable
> >> > themself, but need to handle supplies for them to work.
> >> >
> >> > In schematics they often can be seen as
> >> >
> >> >          ----------------
> >> > Enable - | 100MHz,3.3V, | - VDD
> >> >          |    3225      |
> >> >    GND - |              | - OUT
> >> >          ----------------
> >> >
> >> > or similar. The enable pin might be separate but can also just be tied
> >> > to the vdd supply, hence it is optional in the binding.
> >> >
> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> >> > ---
> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >> >  1 file changed, 49 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > new file mode 100644
> >> > index 0000000000000..8bff6b0fd582e
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > @@ -0,0 +1,49 @@
> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> > +%YAML 1.2
> >> > +---
> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> > +
> >> > +title: Voltage controlled oscillator
> >> 
> >> Voltage controlled oscillator? Really? That sounds far too similar to 
> >> a
> >> VCO to me, and the input voltage here (according to the description at
> >> least) does not affect the frequency of oscillation.
> > 
> > That naming was suggested by Stephen in v1 [0] .
> > 
> > Of course the schematics for the board I have only describe it as
> > "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
> > only mentions "supply voltage" in their datasheets but not a dependency
> > between rate and voltage.
> > 
> > [0] 
> > https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> > 
> >> Why the dedicated binding, rather than adding a supply and enable-gpio
> >> to the existing "fixed-clock" binding? I suspect that a large portion 
> >> of
> >> "fixed-clock"s actually require a supply that is (effectively)
> >> always-on.
> > 
> > I guess there are three aspects:
> > - I do remember discussions in the past about not extending generic
> >   bindings with device-specific stuff. I think generic power-sequences
> >   were the topic back then, though that might have changed over time?
> > - There are places that describe "fixed-clock" as
> >   "basic fixed-rate clock that cannot gate" [1]
> > - Stephen also suggested a separate binding [2]
> > 
> > With the fixed-clock being sort of the root for everything else on most
> > systems, I opted to leave it alone. I guess if the consenus really is 
> > that
> > this should go there, I can move it, but discussion in v1
> > 
> > Interestingly the fixed clock had a gpios property 10 years ago [3] :-) 
> > .
> > 
> > [1] 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
> > [2] 
> > https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
> > [3] 
> > https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t
> 
> After finally going through the v1 discussion [4] in detail,
> here are my further thoughts:
> 
> - I agree with dropping the Diodes stuff, [5] because I see
>    no need for that at this point;  though, am I missing
>    something, where are they actually used?

On the Rock5 ITX that 100MHz clock comes the one single oscillator thing.

The Diodes parts are not root sources for their clocks but instead sort
PLLs or something, though their manual describes them as
"clock generator supporting PCI Express and Ethernet requirements" [8]

So they generate the 100MHz (frequency actually is
selected by input pins of the chip) from a separate 25MHz source clock.

One example are the Theobroma/Cherry embedded boards changed in v1 where
they currently are described via existing generic things (no schematics).

Another user is the rk3568-rock3b for example, where the diodes part
is enabled by the same rail as the port itself, so in contrast to the
Rock 5 ITX, it works "by accident" on the rock 3b [9]


The interesting part of the Diodes ICs is that they actually allow
configuration of the generated frequency via their S0 + S1 pins,
though they might be statically set in the board layout without
being user configurable. (Rock3b does it this way for example)


> - I agree that "enable-gpios" and "vdd-supply" should be
>    required in the binding, [5] because that's the basis for
>    something to be actually represented this way

I would only require the vdd supply though.

On the Rock 5 ITX, the chip does have an enable-gpio input, but that is
tied directly to the voltage rail and is not user controllable.


> - I agree that it should be better not to touch fixed-clock
>    at this point, simply because it's used in very many places,
>    and because in this case we need more than it requires (see
>    the bullet point above)
> 
> - As I wrote already, [6] I highly disagree with this being
>    called voltage-controlled oscillator (VCO), [7] simply
>    because it isn't a VCO, but a clock that can be gated;
>    though, looking forward to what the last bullet point
>    asks to be clarified further
> 
> - I still think that gated-clock is the best choice for the
>    name, because it uses "clock" that's used throughout the
>    entire codebase, and uses "gated" to reflect the nature
>    of the clock generator

"gated-oscillator" perhaps? 

This would make it more explicit that we're talking about a root
for clock signals. "gated-clock" can be anything, in the middle
of a clock tree. Having a very generic name, also invites misuse.


> - Maybe we could actually use fixed-gated-clock as the name,
>    which would make more sense from the stanpoint of possibly
>    merging it into fixed-clock at some point, but I'd like
>    to hear first what's actually going on with the Diodes
>    stuff that was deleted in v2, which I already asked about
>    in the first bullet point
> 
> [4] 
> https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
> [5] 
> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
> [6] 
> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
> [7] 
> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/

[8] https://www.diodes.com/assets/Datasheets/PI6C557-03.pdf
[9] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
    page 31, bottom left of the page


> >> > +
> >> > +maintainers:
> >> > +  - Heiko Stuebner <heiko@sntech.de>
> >> > +
> >> > +properties:
> >> > +  compatible:
> >> > +    const: voltage-oscillator
> >> > +
> >> > +  "#clock-cells":
> >> > +    const: 0
> >> > +
> >> > +  clock-frequency: true
> >> > +
> >> > +  clock-output-names:
> >> > +    maxItems: 1
> >> > +
> >> > +  enable-gpios:
> >> > +    description:
> >> > +      Contains a single GPIO specifier for the GPIO that enables and disables
> >> > +      the oscillator.
> >> > +    maxItems: 1
> >> > +
> >> > +  vdd-supply:
> >> > +    description: handle of the regulator that provides the supply voltage
> >> > +
> >> > +required:
> >> > +  - compatible
> >> > +  - "#clock-cells"
> >> > +  - clock-frequency
> >> > +  - vdd-supply
> >> > +
> >> > +additionalProperties: false
> >> > +
> >> > +examples:
> >> > +  - |
> >> > +    voltage-oscillator {
> >> > +      compatible = "voltage-oscillator";
> >> > +      #clock-cells = <0>;
> >> > +      clock-frequency = <1000000000>;
> >> > +      vdd-supply = <&reg_vdd>;
> >> > +    };
> >> > +...
> 

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Heiko,

On 2024-07-18 13:30, Heiko Stübner wrote:
> Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:
>> On 2024-07-18 11:25, Heiko Stübner wrote:
>> > Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
>> >> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
>> >> > In contrast to fixed clocks that are described as ungateable, boards
>> >> > sometimes use additional oscillators for things like PCIe reference
>> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
>> >> > toggled for them to work.
>> >> >
>> >> > This adds a binding for such oscillators that are not configurable
>> >> > themself, but need to handle supplies for them to work.
>> >> >
>> >> > In schematics they often can be seen as
>> >> >
>> >> >          ----------------
>> >> > Enable - | 100MHz,3.3V, | - VDD
>> >> >          |    3225      |
>> >> >    GND - |              | - OUT
>> >> >          ----------------
>> >> >
>> >> > or similar. The enable pin might be separate but can also just be tied
>> >> > to the vdd supply, hence it is optional in the binding.
>> >> >
>> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> >> > ---
>> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>> >> >  1 file changed, 49 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> > new file mode 100644
>> >> > index 0000000000000..8bff6b0fd582e
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> > @@ -0,0 +1,49 @@
>> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> > +%YAML 1.2
>> >> > +---
>> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> > +
>> >> > +title: Voltage controlled oscillator
>> >>
>> >> Voltage controlled oscillator? Really? That sounds far too similar to
>> >> a
>> >> VCO to me, and the input voltage here (according to the description at
>> >> least) does not affect the frequency of oscillation.
>> >
>> > That naming was suggested by Stephen in v1 [0] .
>> >
>> > Of course the schematics for the board I have only describe it as
>> > "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
>> > only mentions "supply voltage" in their datasheets but not a dependency
>> > between rate and voltage.
>> >
>> > [0]
>> > https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
>> >
>> >> Why the dedicated binding, rather than adding a supply and enable-gpio
>> >> to the existing "fixed-clock" binding? I suspect that a large portion
>> >> of
>> >> "fixed-clock"s actually require a supply that is (effectively)
>> >> always-on.
>> >
>> > I guess there are three aspects:
>> > - I do remember discussions in the past about not extending generic
>> >   bindings with device-specific stuff. I think generic power-sequences
>> >   were the topic back then, though that might have changed over time?
>> > - There are places that describe "fixed-clock" as
>> >   "basic fixed-rate clock that cannot gate" [1]
>> > - Stephen also suggested a separate binding [2]
>> >
>> > With the fixed-clock being sort of the root for everything else on most
>> > systems, I opted to leave it alone. I guess if the consenus really is
>> > that
>> > this should go there, I can move it, but discussion in v1
>> >
>> > Interestingly the fixed clock had a gpios property 10 years ago [3] :-)
>> > .
>> >
>> > [1]
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
>> > [2]
>> > https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
>> > [3]
>> > https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t
>> 
>> After finally going through the v1 discussion [4] in detail,
>> here are my further thoughts:
>> 
>> - I agree with dropping the Diodes stuff, [5] because I see
>>    no need for that at this point;  though, am I missing
>>    something, where are they actually used?
> 
> On the Rock5 ITX that 100MHz clock comes the one single oscillator 
> thing.
> 
> The Diodes parts are not root sources for their clocks but instead sort
> PLLs or something, though their manual describes them as
> "clock generator supporting PCI Express and Ethernet requirements" [8]
> 
> So they generate the 100MHz (frequency actually is
> selected by input pins of the chip) from a separate 25MHz source clock.
> 
> One example are the Theobroma/Cherry embedded boards changed in v1 
> where
> they currently are described via existing generic things (no 
> schematics).
> 
> Another user is the rk3568-rock3b for example, where the diodes part
> is enabled by the same rail as the port itself, so in contrast to the
> Rock 5 ITX, it works "by accident" on the rock 3b [9]

Ah, I see now, thanks for the clarification.  The way Diodes PI6C557
is used on the ROCK 3B, together with its 25 MHz "passive" crystal,
is pretty much the same as the way Aurasemi AU5426 is used on the
ROCK 5 ITX, together with its 100 MHz "active" clock generator.  All
that from the software standpoint, of course.

To explain it further, the PI6C577 and the AU5426 are the components
that actually generate the clocks for the PCIe interfaces.  Thus,
technically we should describe two components per board in their DTs:

- ROCK 5 ITX:
     - 100 MHz clock generator (which goes to the AU5426),
       i.e. the "100MHz,3.3V,3225"
     - clock buffer that fans the 100 MHz clock out to the
       PCIe interfaces, i.e. the Aurasemi AU5426

- ROCK 3B:
     - 25 MHz "passive" crystal (which goes to the PI6C557)
     - clock generator that uses the 25 MHz crystal to generate
       100 MHz and fan it out to the PCIe interfaces, i.e. the
       Diodes PI6C557

(The "passive" 25 MHz crystal is very similar to the main 24 MHz
crystal used by the SoC, also known as xin24m.)

However, simplifying and abstracting the things out a bit should
be fine, to end up with the following:

- ROCK 5 ITX:
     - "black box" that generates fixed 100 MHz clocks for the
        PCIe interfaces, which can be gated

- ROCK 3B:
     - "black box" that generates fixed 100 MHz clocks for the
        PCIe interfaces, which can be gated

... and that's our, hopefully, gated-clock. :)

> The interesting part of the Diodes ICs is that they actually allow
> configuration of the generated frequency via their S0 + S1 pins,
> though they might be statically set in the board layout without
> being user configurable. (Rock3b does it this way for example)

The Aurasemi AU5426 also has a few tricks up its sleeve. :)  For
example, it can also use a "passive" crystal instead of using an
external clock generator, i.e. it can be configured to work in
the same "big picture" layout as the PI6C577.

>> - I agree that "enable-gpios" and "vdd-supply" should be
>>    required in the binding, [5] because that's the basis for
>>    something to be actually represented this way
> 
> I would only require the vdd supply though.
> 
> On the Rock 5 ITX, the chip does have an enable-gpio input, but that is
> tied directly to the voltage rail and is not user controllable.

Isn't that voltage rail (VCC3V3_PI6C_05, provided by the U90099 
regulator,
which is an RT9193) actually enabled by GPIO1_A4_d (see pages 14 and 35
in the ROCK 5 ITX schematic and follow PCIE30x_PWREN_H)?

Unless I'm missing something, that's the source of all troubles, because
we basically also need to enable PCIE30x_PWREN_H when only the other M.2
slot is in use, or when it's enumerated first.  Thus, we need 
enable-gpios
to be able to enable the VCC3V3_PI6C_05 independently.

>> - I agree that it should be better not to touch fixed-clock
>>    at this point, simply because it's used in very many places,
>>    and because in this case we need more than it requires (see
>>    the bullet point above)
>> 
>> - As I wrote already, [6] I highly disagree with this being
>>    called voltage-controlled oscillator (VCO), [7] simply
>>    because it isn't a VCO, but a clock that can be gated;
>>    though, looking forward to what the last bullet point
>>    asks to be clarified further
>> 
>> - I still think that gated-clock is the best choice for the
>>    name, because it uses "clock" that's used throughout the
>>    entire codebase, and uses "gated" to reflect the nature
>>    of the clock generator
> 
> "gated-oscillator" perhaps?

But what's the output of an oscillator, which we actually care about?
It's nothing more but a clock. :)

> This would make it more explicit that we're talking about a root
> for clock signals. "gated-clock" can be anything, in the middle
> of a clock tree. Having a very generic name, also invites misuse.

I can't escape wondering why doesn't "fixed-clock", which is also
a very generic name, invite abuse?

Speaking about the root of clock signals, the above-mentioned xin24m,
which is represented in DTs as a fixed-clock, is the root of pretty
much everything.  Also, it technically isn't a clock, it's a crystal.
That's another example of the above-mentioned abstraction, in which
we care about the way sotware sees it, which is a clock.

>> - Maybe we could actually use fixed-gated-clock as the name,
>>    which would make more sense from the stanpoint of possibly
>>    merging it into fixed-clock at some point, but I'd like
>>    to hear first what's actually going on with the Diodes
>>    stuff that was deleted in v2, which I already asked about
>>    in the first bullet point
>> 
>> [4]
>> https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
>> [5]
>> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
>> [6]
>> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
>> [7]
>> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> 
> [8] https://www.diodes.com/assets/Datasheets/PI6C557-03.pdf
> [9] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
>     page 31, bottom left of the page
> 
> 
>> >> > +
>> >> > +maintainers:
>> >> > +  - Heiko Stuebner <heiko@sntech.de>
>> >> > +
>> >> > +properties:
>> >> > +  compatible:
>> >> > +    const: voltage-oscillator
>> >> > +
>> >> > +  "#clock-cells":
>> >> > +    const: 0
>> >> > +
>> >> > +  clock-frequency: true
>> >> > +
>> >> > +  clock-output-names:
>> >> > +    maxItems: 1
>> >> > +
>> >> > +  enable-gpios:
>> >> > +    description:
>> >> > +      Contains a single GPIO specifier for the GPIO that enables and disables
>> >> > +      the oscillator.
>> >> > +    maxItems: 1
>> >> > +
>> >> > +  vdd-supply:
>> >> > +    description: handle of the regulator that provides the supply voltage
>> >> > +
>> >> > +required:
>> >> > +  - compatible
>> >> > +  - "#clock-cells"
>> >> > +  - clock-frequency
>> >> > +  - vdd-supply
>> >> > +
>> >> > +additionalProperties: false
>> >> > +
>> >> > +examples:
>> >> > +  - |
>> >> > +    voltage-oscillator {
>> >> > +      compatible = "voltage-oscillator";
>> >> > +      #clock-cells = <0>;
>> >> > +      clock-frequency = <1000000000>;
>> >> > +      vdd-supply = <&reg_vdd>;
>> >> > +    };
>> >> > +...

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Hi Dragan,

Am Donnerstag, 18. Juli 2024, 15:00:57 CEST schrieb Dragan Simic:
> On 2024-07-18 13:30, Heiko Stübner wrote:
> > Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:
> >> On 2024-07-18 11:25, Heiko Stübner wrote:
> >> > Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
> >> >> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> >> >> > In contrast to fixed clocks that are described as ungateable, boards
> >> >> > sometimes use additional oscillators for things like PCIe reference
> >> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
> >> >> > toggled for them to work.
> >> >> >
> >> >> > This adds a binding for such oscillators that are not configurable
> >> >> > themself, but need to handle supplies for them to work.
> >> >> >
> >> >> > In schematics they often can be seen as
> >> >> >
> >> >> >          ----------------
> >> >> > Enable - | 100MHz,3.3V, | - VDD
> >> >> >          |    3225      |
> >> >> >    GND - |              | - OUT
> >> >> >          ----------------
> >> >> >
> >> >> > or similar. The enable pin might be separate but can also just be tied
> >> >> > to the vdd supply, hence it is optional in the binding.
> >> >> >
> >> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> >> >> > ---
> >> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >> >> >  1 file changed, 49 insertions(+)
> >> >> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >> >
> >> >> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >> > new file mode 100644
> >> >> > index 0000000000000..8bff6b0fd582e
> >> >> > --- /dev/null
> >> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >> > @@ -0,0 +1,49 @@
> >> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> >> > +%YAML 1.2
> >> >> > +---
> >> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> >> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> >> > +
> >> >> > +title: Voltage controlled oscillator
> >> >>
> >> >> Voltage controlled oscillator? Really? That sounds far too similar to
> >> >> a
> >> >> VCO to me, and the input voltage here (according to the description at
> >> >> least) does not affect the frequency of oscillation.
> >> >
> >> > That naming was suggested by Stephen in v1 [0] .
> >> >
> >> > Of course the schematics for the board I have only describe it as
> >> > "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
> >> > only mentions "supply voltage" in their datasheets but not a dependency
> >> > between rate and voltage.
> >> >
> >> > [0]
> >> > https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> >> >
> >> >> Why the dedicated binding, rather than adding a supply and enable-gpio
> >> >> to the existing "fixed-clock" binding? I suspect that a large portion
> >> >> of
> >> >> "fixed-clock"s actually require a supply that is (effectively)
> >> >> always-on.
> >> >
> >> > I guess there are three aspects:
> >> > - I do remember discussions in the past about not extending generic
> >> >   bindings with device-specific stuff. I think generic power-sequences
> >> >   were the topic back then, though that might have changed over time?
> >> > - There are places that describe "fixed-clock" as
> >> >   "basic fixed-rate clock that cannot gate" [1]
> >> > - Stephen also suggested a separate binding [2]
> >> >
> >> > With the fixed-clock being sort of the root for everything else on most
> >> > systems, I opted to leave it alone. I guess if the consenus really is
> >> > that
> >> > this should go there, I can move it, but discussion in v1
> >> >
> >> > Interestingly the fixed clock had a gpios property 10 years ago [3] :-)
> >> > .
> >> >
> >> > [1]
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
> >> > [2]
> >> > https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
> >> > [3]
> >> > https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t
> >> 
> >> After finally going through the v1 discussion [4] in detail,
> >> here are my further thoughts:
> >> 
> >> - I agree with dropping the Diodes stuff, [5] because I see
> >>    no need for that at this point;  though, am I missing
> >>    something, where are they actually used?
> > 
> > On the Rock5 ITX that 100MHz clock comes the one single oscillator 
> > thing.
> > 
> > The Diodes parts are not root sources for their clocks but instead sort
> > PLLs or something, though their manual describes them as
> > "clock generator supporting PCI Express and Ethernet requirements" [8]
> > 
> > So they generate the 100MHz (frequency actually is
> > selected by input pins of the chip) from a separate 25MHz source clock.
> > 
> > One example are the Theobroma/Cherry embedded boards changed in v1 
> > where
> > they currently are described via existing generic things (no 
> > schematics).
> > 
> > Another user is the rk3568-rock3b for example, where the diodes part
> > is enabled by the same rail as the port itself, so in contrast to the
> > Rock 5 ITX, it works "by accident" on the rock 3b [9]
> 
> Ah, I see now, thanks for the clarification.  The way Diodes PI6C557
> is used on the ROCK 3B, together with its 25 MHz "passive" crystal,
> is pretty much the same as the way Aurasemi AU5426 is used on the
> ROCK 5 ITX, together with its 100 MHz "active" clock generator.  All
> that from the software standpoint, of course.
> 
> To explain it further, the PI6C577 and the AU5426 are the components
> that actually generate the clocks for the PCIe interfaces.  Thus,
> technically we should describe two components per board in their DTs:
> 
> - ROCK 5 ITX:
>      - 100 MHz clock generator (which goes to the AU5426),
>        i.e. the "100MHz,3.3V,3225"
>      - clock buffer that fans the 100 MHz clock out to the
>        PCIe interfaces, i.e. the Aurasemi AU5426
> 
> - ROCK 3B:
>      - 25 MHz "passive" crystal (which goes to the PI6C557)
>      - clock generator that uses the 25 MHz crystal to generate
>        100 MHz and fan it out to the PCIe interfaces, i.e. the
>        Diodes PI6C557
> 
> (The "passive" 25 MHz crystal is very similar to the main 24 MHz
> crystal used by the SoC, also known as xin24m.)
> 
> However, simplifying and abstracting the things out a bit should
> be fine, to end up with the following:
> 
> - ROCK 5 ITX:
>      - "black box" that generates fixed 100 MHz clocks for the
>         PCIe interfaces, which can be gated
> 
> - ROCK 3B:
>      - "black box" that generates fixed 100 MHz clocks for the
>         PCIe interfaces, which can be gated
> 
> ... and that's our, hopefully, gated-clock. :)

I don't think we want too many black-boxes. Devicetree's purpose is to
describe the hardware as best as possible, the OS can make of it what it
wants afterwards ;-) .

That was also Stephen's point in v1 when he wanted to distinguish
between the input-less oscillator and the Diodes things that needs an
input clock.


> > The interesting part of the Diodes ICs is that they actually allow
> > configuration of the generated frequency via their S0 + S1 pins,
> > though they might be statically set in the board layout without
> > being user configurable. (Rock3b does it this way for example)
> 
> The Aurasemi AU5426 also has a few tricks up its sleeve. :)  For
> example, it can also use a "passive" crystal instead of using an
> external clock generator, i.e. it can be configured to work in
> the same "big picture" layout as the PI6C577.
> 
> >> - I agree that "enable-gpios" and "vdd-supply" should be
> >>    required in the binding, [5] because that's the basis for
> >>    something to be actually represented this way
> > 
> > I would only require the vdd supply though.
> > 
> > On the Rock 5 ITX, the chip does have an enable-gpio input, but that is
> > tied directly to the voltage rail and is not user controllable.
> 
> Isn't that voltage rail (VCC3V3_PI6C_05, provided by the U90099 
> regulator,
> which is an RT9193) actually enabled by GPIO1_A4_d (see pages 14 and 35
> in the ROCK 5 ITX schematic and follow PCIE30x_PWREN_H)?
> 
> Unless I'm missing something, that's the source of all troubles, because
> we basically also need to enable PCIE30x_PWREN_H when only the other M.2
> slot is in use, or when it's enumerated first.  Thus, we need 
> enable-gpios
> to be able to enable the VCC3V3_PI6C_05 independently.

And you correctly described the problem I faced and that got me on that
journey (sata-pcie-controller probing before M.2 attached controller).

PCIE30x4_PWREN_H actually drives two separate regulators though,
U90098 and U90099 . 

U90098 creates VCC3V3_MKEY which is the supply for the main M.2 slot
U90099 creates VCC3V3_PI6C_05 which is the oscillator and Au5426 supply

PCIE30x4_PWREN_H definitly belongs to a regulator node in the devicetree
that then either the M.2 slot or the oscillator can enable first.


> >> - I agree that it should be better not to touch fixed-clock
> >>    at this point, simply because it's used in very many places,
> >>    and because in this case we need more than it requires (see
> >>    the bullet point above)
> >> 
> >> - As I wrote already, [6] I highly disagree with this being
> >>    called voltage-controlled oscillator (VCO), [7] simply
> >>    because it isn't a VCO, but a clock that can be gated;
> >>    though, looking forward to what the last bullet point
> >>    asks to be clarified further
> >> 
> >> - I still think that gated-clock is the best choice for the
> >>    name, because it uses "clock" that's used throughout the
> >>    entire codebase, and uses "gated" to reflect the nature
> >>    of the clock generator
> > 
> > "gated-oscillator" perhaps?
> 
> But what's the output of an oscillator, which we actually care about?
> It's nothing more but a clock. :)
> 
> > This would make it more explicit that we're talking about a root
> > for clock signals. "gated-clock" can be anything, in the middle
> > of a clock tree. Having a very generic name, also invites misuse.
> 
> I can't escape wondering why doesn't "fixed-clock", which is also
> a very generic name, invite abuse?

I guess it does - see the misuse in the rk3588 tiger and jaguar boards.
But it is sort of grandfathered I guess. It is still from a time more than
12 years ago, when architectures (like sunxi) tried to model their clock-
controllers via individual interconnected dt-nodes.


> Speaking about the root of clock signals, the above-mentioned xin24m,
> which is represented in DTs as a fixed-clock, is the root of pretty
> much everything.  Also, it technically isn't a clock, it's a crystal.
> That's another example of the above-mentioned abstraction, in which
> we care about the way sotware sees it, which is a clock.

At this point I think we should dt-people weigh in more on which
direction to take ;-) .

Because as a lot of dt-binding review in the past mentioned, the
"software-perspective" doesn't matter for the binding.


Heiko

> >> - Maybe we could actually use fixed-gated-clock as the name,
> >>    which would make more sense from the stanpoint of possibly
> >>    merging it into fixed-clock at some point, but I'd like
> >>    to hear first what's actually going on with the Diodes
> >>    stuff that was deleted in v2, which I already asked about
> >>    in the first bullet point
> >> 
> >> [4]
> >> https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
> >> [5]
> >> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
> >> [6]
> >> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
> >> [7]
> >> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> > 
> > [8] https://www.diodes.com/assets/Datasheets/PI6C557-03.pdf
> > [9] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
> >     page 31, bottom left of the page
> > 
> > 
> >> >> > +
> >> >> > +maintainers:
> >> >> > +  - Heiko Stuebner <heiko@sntech.de>
> >> >> > +
> >> >> > +properties:
> >> >> > +  compatible:
> >> >> > +    const: voltage-oscillator
> >> >> > +
> >> >> > +  "#clock-cells":
> >> >> > +    const: 0
> >> >> > +
> >> >> > +  clock-frequency: true
> >> >> > +
> >> >> > +  clock-output-names:
> >> >> > +    maxItems: 1
> >> >> > +
> >> >> > +  enable-gpios:
> >> >> > +    description:
> >> >> > +      Contains a single GPIO specifier for the GPIO that enables and disables
> >> >> > +      the oscillator.
> >> >> > +    maxItems: 1
> >> >> > +
> >> >> > +  vdd-supply:
> >> >> > +    description: handle of the regulator that provides the supply voltage
> >> >> > +
> >> >> > +required:
> >> >> > +  - compatible
> >> >> > +  - "#clock-cells"
> >> >> > +  - clock-frequency
> >> >> > +  - vdd-supply
> >> >> > +
> >> >> > +additionalProperties: false
> >> >> > +
> >> >> > +examples:
> >> >> > +  - |
> >> >> > +    voltage-oscillator {
> >> >> > +      compatible = "voltage-oscillator";
> >> >> > +      #clock-cells = <0>;
> >> >> > +      clock-frequency = <1000000000>;
> >> >> > +      vdd-supply = <&reg_vdd>;
> >> >> > +    };
> >> >> > +...
> 

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Heiko,

On 2024-07-18 15:50, Heiko Stübner wrote:
> Am Donnerstag, 18. Juli 2024, 15:00:57 CEST schrieb Dragan Simic:
>> On 2024-07-18 13:30, Heiko Stübner wrote:
>> > Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:
>> >> On 2024-07-18 11:25, Heiko Stübner wrote:
>> >> > Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
>> >> >> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
>> >> >> > In contrast to fixed clocks that are described as ungateable, boards
>> >> >> > sometimes use additional oscillators for things like PCIe reference
>> >> >> > clocks, that need actual supplies to get enabled and enable-gpios to be
>> >> >> > toggled for them to work.
>> >> >> >
>> >> >> > This adds a binding for such oscillators that are not configurable
>> >> >> > themself, but need to handle supplies for them to work.
>> >> >> >
>> >> >> > In schematics they often can be seen as
>> >> >> >
>> >> >> >          ----------------
>> >> >> > Enable - | 100MHz,3.3V, | - VDD
>> >> >> >          |    3225      |
>> >> >> >    GND - |              | - OUT
>> >> >> >          ----------------
>> >> >> >
>> >> >> > or similar. The enable pin might be separate but can also just be tied
>> >> >> > to the vdd supply, hence it is optional in the binding.
>> >> >> >
>> >> >> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> >> >> > ---
>> >> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
>> >> >> >  1 file changed, 49 insertions(+)
>> >> >> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> >> >
>> >> >> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> >> > new file mode 100644
>> >> >> > index 0000000000000..8bff6b0fd582e
>> >> >> > --- /dev/null
>> >> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
>> >> >> > @@ -0,0 +1,49 @@
>> >> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> >> > +%YAML 1.2
>> >> >> > +---
>> >> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
>> >> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> >> > +
>> >> >> > +title: Voltage controlled oscillator
>> >> >>
>> >> >> Voltage controlled oscillator? Really? That sounds far too similar to
>> >> >> a
>> >> >> VCO to me, and the input voltage here (according to the description at
>> >> >> least) does not affect the frequency of oscillation.
>> >> >
>> >> > That naming was suggested by Stephen in v1 [0] .
>> >> >
>> >> > Of course the schematics for the board I have only describe it as
>> >> > "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
>> >> > only mentions "supply voltage" in their datasheets but not a dependency
>> >> > between rate and voltage.
>> >> >
>> >> > [0]
>> >> > https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
>> >> >
>> >> >> Why the dedicated binding, rather than adding a supply and enable-gpio
>> >> >> to the existing "fixed-clock" binding? I suspect that a large portion
>> >> >> of
>> >> >> "fixed-clock"s actually require a supply that is (effectively)
>> >> >> always-on.
>> >> >
>> >> > I guess there are three aspects:
>> >> > - I do remember discussions in the past about not extending generic
>> >> >   bindings with device-specific stuff. I think generic power-sequences
>> >> >   were the topic back then, though that might have changed over time?
>> >> > - There are places that describe "fixed-clock" as
>> >> >   "basic fixed-rate clock that cannot gate" [1]
>> >> > - Stephen also suggested a separate binding [2]
>> >> >
>> >> > With the fixed-clock being sort of the root for everything else on most
>> >> > systems, I opted to leave it alone. I guess if the consenus really is
>> >> > that
>> >> > this should go there, I can move it, but discussion in v1
>> >> >
>> >> > Interestingly the fixed clock had a gpios property 10 years ago [3] :-)
>> >> > .
>> >> >
>> >> > [1]
>> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
>> >> > [2]
>> >> > https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
>> >> > [3]
>> >> > https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t
>> >>
>> >> After finally going through the v1 discussion [4] in detail,
>> >> here are my further thoughts:
>> >>
>> >> - I agree with dropping the Diodes stuff, [5] because I see
>> >>    no need for that at this point;  though, am I missing
>> >>    something, where are they actually used?
>> >
>> > On the Rock5 ITX that 100MHz clock comes the one single oscillator
>> > thing.
>> >
>> > The Diodes parts are not root sources for their clocks but instead sort
>> > PLLs or something, though their manual describes them as
>> > "clock generator supporting PCI Express and Ethernet requirements" [8]
>> >
>> > So they generate the 100MHz (frequency actually is
>> > selected by input pins of the chip) from a separate 25MHz source clock.
>> >
>> > One example are the Theobroma/Cherry embedded boards changed in v1
>> > where
>> > they currently are described via existing generic things (no
>> > schematics).
>> >
>> > Another user is the rk3568-rock3b for example, where the diodes part
>> > is enabled by the same rail as the port itself, so in contrast to the
>> > Rock 5 ITX, it works "by accident" on the rock 3b [9]
>> 
>> Ah, I see now, thanks for the clarification.  The way Diodes PI6C557
>> is used on the ROCK 3B, together with its 25 MHz "passive" crystal,
>> is pretty much the same as the way Aurasemi AU5426 is used on the
>> ROCK 5 ITX, together with its 100 MHz "active" clock generator.  All
>> that from the software standpoint, of course.
>> 
>> To explain it further, the PI6C577 and the AU5426 are the components
>> that actually generate the clocks for the PCIe interfaces.  Thus,
>> technically we should describe two components per board in their DTs:
>> 
>> - ROCK 5 ITX:
>>      - 100 MHz clock generator (which goes to the AU5426),
>>        i.e. the "100MHz,3.3V,3225"
>>      - clock buffer that fans the 100 MHz clock out to the
>>        PCIe interfaces, i.e. the Aurasemi AU5426
>> 
>> - ROCK 3B:
>>      - 25 MHz "passive" crystal (which goes to the PI6C557)
>>      - clock generator that uses the 25 MHz crystal to generate
>>        100 MHz and fan it out to the PCIe interfaces, i.e. the
>>        Diodes PI6C557
>> 
>> (The "passive" 25 MHz crystal is very similar to the main 24 MHz
>> crystal used by the SoC, also known as xin24m.)
>> 
>> However, simplifying and abstracting the things out a bit should
>> be fine, to end up with the following:
>> 
>> - ROCK 5 ITX:
>>      - "black box" that generates fixed 100 MHz clocks for the
>>         PCIe interfaces, which can be gated
>> 
>> - ROCK 3B:
>>      - "black box" that generates fixed 100 MHz clocks for the
>>         PCIe interfaces, which can be gated
>> 
>> ... and that's our, hopefully, gated-clock. :)
> 
> I don't think we want too many black-boxes. Devicetree's purpose is to
> describe the hardware as best as possible, the OS can make of it what 
> it
> wants afterwards ;-) .
> 
> That was also Stephen's point in v1 when he wanted to distinguish
> between the input-less oscillator and the Diodes things that needs an
> input clock.

I can agree with that for sure, but in that case, we'd need much
more than just one "black box" per board dtb, namely, and by reusing
and expanding a bit one of the earlier lists:

- ROCK 5 ITX:
      - gated-clock
        (100 MHz clock generator (which goes to the AU5426),
        i.e. the "100MHz,3.3V,3225")
        - clock-fanout
          (clock buffer that fans the 100 MHz clock out to the
          PCIe interfaces, i.e. the Aurasemi AU5426)

- ROCK 3B:
      - fixed-clock
        (25 MHz "passive" crystal (which goes to the PI6C557))
        - clock-fanout
          (clock generator that uses the 25 MHz crystal to generate
          100 MHz and fan it out to the PCIe interfaces, i.e. the
          Diodes PI6C557)

Actually, I'd be happy to see such a precise hardware definition
in these (and some other) board dts files. :)

>> > The interesting part of the Diodes ICs is that they actually allow
>> > configuration of the generated frequency via their S0 + S1 pins,
>> > though they might be statically set in the board layout without
>> > being user configurable. (Rock3b does it this way for example)
>> 
>> The Aurasemi AU5426 also has a few tricks up its sleeve. :)  For
>> example, it can also use a "passive" crystal instead of using an
>> external clock generator, i.e. it can be configured to work in
>> the same "big picture" layout as the PI6C577.
>> 
>> >> - I agree that "enable-gpios" and "vdd-supply" should be
>> >>    required in the binding, [5] because that's the basis for
>> >>    something to be actually represented this way
>> >
>> > I would only require the vdd supply though.
>> >
>> > On the Rock 5 ITX, the chip does have an enable-gpio input, but that is
>> > tied directly to the voltage rail and is not user controllable.
>> 
>> Isn't that voltage rail (VCC3V3_PI6C_05, provided by the U90099
>> regulator,
>> which is an RT9193) actually enabled by GPIO1_A4_d (see pages 14 and 
>> 35
>> in the ROCK 5 ITX schematic and follow PCIE30x_PWREN_H)?
>> 
>> Unless I'm missing something, that's the source of all troubles, 
>> because
>> we basically also need to enable PCIE30x_PWREN_H when only the other 
>> M.2
>> slot is in use, or when it's enumerated first.  Thus, we need
>> enable-gpios
>> to be able to enable the VCC3V3_PI6C_05 independently.
> 
> And you correctly described the problem I faced and that got me on that
> journey (sata-pcie-controller probing before M.2 attached controller).
> 
> PCIE30x4_PWREN_H actually drives two separate regulators though,
> U90098 and U90099 .
> 
> U90098 creates VCC3V3_MKEY which is the supply for the main M.2 slot
> U90099 creates VCC3V3_PI6C_05 which is the oscillator and Au5426 supply
> 
> PCIE30x4_PWREN_H definitly belongs to a regulator node in the 
> devicetree
> that then either the M.2 slot or the oscillator can enable first.

Exactly, PCIE30x4_PWREN_H basically plays a double role, so it needs
to be enabled idenpendently in each of its roles, for the observed
problem to be resolved.

>> >> - I agree that it should be better not to touch fixed-clock
>> >>    at this point, simply because it's used in very many places,
>> >>    and because in this case we need more than it requires (see
>> >>    the bullet point above)
>> >>
>> >> - As I wrote already, [6] I highly disagree with this being
>> >>    called voltage-controlled oscillator (VCO), [7] simply
>> >>    because it isn't a VCO, but a clock that can be gated;
>> >>    though, looking forward to what the last bullet point
>> >>    asks to be clarified further
>> >>
>> >> - I still think that gated-clock is the best choice for the
>> >>    name, because it uses "clock" that's used throughout the
>> >>    entire codebase, and uses "gated" to reflect the nature
>> >>    of the clock generator
>> >
>> > "gated-oscillator" perhaps?
>> 
>> But what's the output of an oscillator, which we actually care about?
>> It's nothing more but a clock. :)
>> 
>> > This would make it more explicit that we're talking about a root
>> > for clock signals. "gated-clock" can be anything, in the middle
>> > of a clock tree. Having a very generic name, also invites misuse.
>> 
>> I can't escape wondering why doesn't "fixed-clock", which is also
>> a very generic name, invite abuse?
> 
> I guess it does - see the misuse in the rk3588 tiger and jaguar boards.
> But it is sort of grandfathered I guess. It is still from a time more 
> than
> 12 years ago, when architectures (like sunxi) tried to model their 
> clock-
> controllers via individual interconnected dt-nodes.

Perhaps, but again, "clock" became the universally used term
throughout the entire codebase.

>> Speaking about the root of clock signals, the above-mentioned xin24m,
>> which is represented in DTs as a fixed-clock, is the root of pretty
>> much everything.  Also, it technically isn't a clock, it's a crystal.
>> That's another example of the above-mentioned abstraction, in which
>> we care about the way sotware sees it, which is a clock.
> 
> At this point I think we should dt-people weigh in more on which
> direction to take ;-) .
> 
> Because as a lot of dt-binding review in the past mentioned, the
> "software-perspective" doesn't matter for the binding.

Looking forward to hearing more opinions. :)

>> >> - Maybe we could actually use fixed-gated-clock as the name,
>> >>    which would make more sense from the stanpoint of possibly
>> >>    merging it into fixed-clock at some point, but I'd like
>> >>    to hear first what's actually going on with the Diodes
>> >>    stuff that was deleted in v2, which I already asked about
>> >>    in the first bullet point
>> >>
>> >> [4]
>> >> https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
>> >> [5]
>> >> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
>> >> [6]
>> >> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
>> >> [7]
>> >> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
>> >
>> > [8] https://www.diodes.com/assets/Datasheets/PI6C557-03.pdf
>> > [9] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
>> >     page 31, bottom left of the page
>> >
>> >
>> >> >> > +
>> >> >> > +maintainers:
>> >> >> > +  - Heiko Stuebner <heiko@sntech.de>
>> >> >> > +
>> >> >> > +properties:
>> >> >> > +  compatible:
>> >> >> > +    const: voltage-oscillator
>> >> >> > +
>> >> >> > +  "#clock-cells":
>> >> >> > +    const: 0
>> >> >> > +
>> >> >> > +  clock-frequency: true
>> >> >> > +
>> >> >> > +  clock-output-names:
>> >> >> > +    maxItems: 1
>> >> >> > +
>> >> >> > +  enable-gpios:
>> >> >> > +    description:
>> >> >> > +      Contains a single GPIO specifier for the GPIO that enables and disables
>> >> >> > +      the oscillator.
>> >> >> > +    maxItems: 1
>> >> >> > +
>> >> >> > +  vdd-supply:
>> >> >> > +    description: handle of the regulator that provides the supply voltage
>> >> >> > +
>> >> >> > +required:
>> >> >> > +  - compatible
>> >> >> > +  - "#clock-cells"
>> >> >> > +  - clock-frequency
>> >> >> > +  - vdd-supply
>> >> >> > +
>> >> >> > +additionalProperties: false
>> >> >> > +
>> >> >> > +examples:
>> >> >> > +  - |
>> >> >> > +    voltage-oscillator {
>> >> >> > +      compatible = "voltage-oscillator";
>> >> >> > +      #clock-cells = <0>;
>> >> >> > +      clock-frequency = <1000000000>;
>> >> >> > +      vdd-supply = <&reg_vdd>;
>> >> >> > +    };
>> >> >> > +...

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4951 bytes --]

On Thu, Jul 18, 2024 at 11:25:28AM +0200, Heiko Stübner wrote:
> Hi Conor,
> 
> Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
> > On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> > > In contrast to fixed clocks that are described as ungateable, boards
> > > sometimes use additional oscillators for things like PCIe reference
> > > clocks, that need actual supplies to get enabled and enable-gpios to be
> > > toggled for them to work.
> > > 
> > > This adds a binding for such oscillators that are not configurable
> > > themself, but need to handle supplies for them to work.
> > > 
> > > In schematics they often can be seen as
> > > 
> > >          ----------------
> > > Enable - | 100MHz,3.3V, | - VDD
> > >          |    3225      |
> > >    GND - |              | - OUT
> > >          ----------------
> > > 
> > > or similar. The enable pin might be separate but can also just be tied
> > > to the vdd supply, hence it is optional in the binding.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > > new file mode 100644
> > > index 0000000000000..8bff6b0fd582e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> > > @@ -0,0 +1,49 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Voltage controlled oscillator
> > 
> > Voltage controlled oscillator? Really? That sounds far too similar to a
> > VCO to me, and the input voltage here (according to the description at
> > least) does not affect the frequency of oscillation.
> 
> That naming was suggested by Stephen in v1 [0] .

I think "voltage-oscillator" is a confusing name, and having "voltage
controlled oscillator" in the title doubly so as this isn't a binding
for a VCO.
A VCO is a more general case of the sort of device that you're talking
about here, so a part of me can see it - but I think specific
compatibles would be required for actual VCOs, since the "transfer
function" would vary per device.

> Of course the schematics for the board I have only describe it as
> "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
> only mentions "supply voltage" in their datasheets but not a dependency
> between rate and voltage.
> 
> [0] https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> 
> > Why the dedicated binding, rather than adding a supply and enable-gpio
> > to the existing "fixed-clock" binding? I suspect that a large portion of
> > "fixed-clock"s actually require a supply that is (effectively)
> > always-on.
> 
> I guess there are three aspects:
> - I do remember discussions in the past about not extending generic
>   bindings with device-specific stuff.

FWIW, I wouldn't classify this as device-specific. "enable-gpios" and
"vdd-supply" are pretty generic and I think the latter is missing from
the vast majority of real* "fixed-clocks". I would expect that devices
where the datasheet would call

* Real because there's plenty of "fixed-clocks" (both in and out of tree)
that are used to work around the lack of a clock-controller driver for an
SoC.

> I think generic power-sequences
>   were the topic back then, though that might have changed over time?
> - There are places that describe "fixed-clock" as
>   "basic fixed-rate clock that cannot gate" [1]

I think that that is something that could be changed, it's "just" a
comment in some code! Sounds like Stephen disagrees though :)

> - Stephen also suggested a separate binding [2]

I liked your "gated-oscillator" suggestion in another reply, but
"gated-fixed-clock" might be a better "thematic" fit since this is a
special case of fixed-clocks?

Cheers,
Conor.

> With the fixed-clock being sort of the root for everything else on most
> systems, I opted to leave it alone. I guess if the consenus really is that
> this should go there, I can move it, but discussion in v1 
> 
> Interestingly the fixed clock had a gpios property 10 years ago [3] :-) .

heh!

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
> [2] https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
> [3] https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Stephen Boyd]

Quoting Conor Dooley (2024-07-18 08:59:50)
> 
> FWIW, I wouldn't classify this as device-specific. "enable-gpios" and
> "vdd-supply" are pretty generic and I think the latter is missing from
> the vast majority of real* "fixed-clocks". I would expect that devices
> where the datasheet would call
> 
> * Real because there's plenty of "fixed-clocks" (both in and out of tree)
> that are used to work around the lack of a clock-controller driver for an
> SoC.

I agree!

> 
> > I think generic power-sequences
> >   were the topic back then, though that might have changed over time?
> > - There are places that describe "fixed-clock" as
> >   "basic fixed-rate clock that cannot gate" [1]
> 
> I think that that is something that could be changed, it's "just" a
> comment in some code! Sounds like Stephen disagrees though :)

It's more about making a clear break from the fixed-clock binding so
that the extra properties are required.

> 
> > - Stephen also suggested a separate binding [2]
> 
> I liked your "gated-oscillator" suggestion in another reply, but
> "gated-fixed-clock" might be a better "thematic" fit since this is a
> special case of fixed-clocks?
> 

It looks to me like we've arrived at the hardest problem in computer
science, i.e. naming. Any of these names is fine. I'd look to see what
those parts on mouser are called and use that to drive the compatible
name decision if you can't decide. The description section in the
binding could be verbose and link to some parts/pdfs if that helps too.
In the past I've seen EEs call these things clock buffers. I'm not a
classically trained EE myself but it usually helps to use similar names
from the schematic in DT because DT authors are sorta translating
schematics to DT.

Re: [PATCH v2 2/3] clk: add driver for voltage controlled oscillators

[Stephen Boyd]

Quoting Heiko Stuebner (2024-07-15 04:02:50)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 4abe16c8ccdfe..ca7b7b7ddfd8d 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_COMMON_CLK_SI521XX)      += clk-si521xx.o
>  obj-$(CONFIG_COMMON_CLK_VC3)           += clk-versaclock3.o
>  obj-$(CONFIG_COMMON_CLK_VC5)           += clk-versaclock5.o
>  obj-$(CONFIG_COMMON_CLK_VC7)           += clk-versaclock7.o
> +obj-$(CONFIG_COMMON_CLK_VCO)           += clk-vco.o

Wrong section. It's basically a common clk type.

>  obj-$(CONFIG_COMMON_CLK_WM831X)                += clk-wm831x.o
>  obj-$(CONFIG_COMMON_CLK_XGENE)         += clk-xgene.o
>  
> diff --git a/drivers/clk/clk-vco.c b/drivers/clk/clk-vco.c
> new file mode 100644
> index 0000000000000..f7fe2bc627f36
> --- /dev/null
> +++ b/drivers/clk/clk-vco.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
> + *
> + * Generic voltage controlled oscillator
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct clk_vco {
> +       struct device *dev;
> +       struct clk_hw hw;
> +       u32 rate;
> +       struct regulator *supply;
> +       struct gpio_desc *enable_gpio;
> +};
> +
> +#define to_clk_vco(_hw) container_of(_hw, struct clk_vco, hw)
> +
> +static int clk_vco_prepare(struct clk_hw *hw)
> +{
> +       return regulator_enable(to_clk_vco(hw)->supply);
> +}
> +
> +static void clk_vco_unprepare(struct clk_hw *hw)
> +{
> +       regulator_disable(to_clk_vco(hw)->supply);
> +}
> +
> +static int clk_vco_enable(struct clk_hw *hw)
> +{
> +       gpiod_set_value(to_clk_vco(hw)->enable_gpio, 1);
> +       return 0;
> +}
> +
> +static void clk_vco_disable(struct clk_hw *hw)
> +{
> +       gpiod_set_value(to_clk_vco(hw)->enable_gpio, 0);
> +}

It looks similar to clk-gpio.c code, but not as complete because it
assumes gpios can't sleep. Please look into reusing that code somehow,
possibly exporting 'clk_gpio_gate_ops' and struct clk_gpio for use in
this new driver. It would be good to fold the sleepable gpio bit as well
somehow, maybe with a new function to get a device's gpiod along with
returning a const pointer to the clk_ops that can be copied and amended
with the regulator part.

> +
> +static unsigned long clk_vco_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long parent_rate)
> +{
> +       return to_clk_vco(hw)->rate;
> +}
> +
> +const struct clk_ops clk_vco_ops = {
> +       .prepare = clk_vco_prepare,
> +       .unprepare = clk_vco_unprepare,
> +       .enable = clk_vco_enable,
> +       .disable = clk_vco_disable,
> +       .recalc_rate = clk_vco_recalc_rate,
> +};
> +
> +static int clk_vco_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct clk_vco *clkgen;
> +       const char *clk_name;
> +       int ret;
> +
> +       clkgen = devm_kzalloc(dev, sizeof(*clkgen), GFP_KERNEL);
> +       if (!clkgen)
> +               return -ENOMEM;
> +
> +       clkgen->dev = dev;

Is this used outside of probe? Why stash it?

> +
> +       if (device_property_read_u32(dev, "clock-frequency", &clkgen->rate))
> +               return dev_err_probe(dev, -EIO, "failed to get clock-frequency");
> +
> +       ret = device_property_read_string(dev, "clock-output-names", &clk_name);
> +       if (ret)
> +               clk_name = fwnode_get_name(dev->fwnode);
> +
> +       clkgen->supply = devm_regulator_get_optional(dev, "vdd");
> +       if (IS_ERR(clkgen->supply)) {
> +               if (PTR_ERR(clkgen->supply) != -ENODEV)
> +                       return dev_err_probe(dev, PTR_ERR(clkgen->supply),
> +                                            "failed to get regulator\n");
> +               clkgen->supply = NULL;
> +       }
> +
> +       clkgen->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> +                                                     GPIOD_OUT_LOW);
> +       if (IS_ERR(clkgen->enable_gpio))
> +               return dev_err_probe(dev, PTR_ERR(clkgen->enable_gpio),
> +                                    "failed to get gpio\n");
> +
> +       ret = gpiod_direction_output(clkgen->enable_gpio, 0);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to set gpio output");

Missing newline.

> +
> +       clkgen->hw.init = CLK_HW_INIT_NO_PARENT(clk_name, &clk_vco_ops, 0);
> +
> +       /* register the clock */
> +       ret = devm_clk_hw_register(dev, &clkgen->hw);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "failed to register clock\n");
> +

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Heiko Stübner]

Am Samstag, 27. Juli 2024, 00:21:44 CEST schrieb Stephen Boyd:
> Quoting Conor Dooley (2024-07-18 08:59:50)
> > 
> > FWIW, I wouldn't classify this as device-specific. "enable-gpios" and
> > "vdd-supply" are pretty generic and I think the latter is missing from
> > the vast majority of real* "fixed-clocks". I would expect that devices
> > where the datasheet would call
> > 
> > * Real because there's plenty of "fixed-clocks" (both in and out of tree)
> > that are used to work around the lack of a clock-controller driver for an
> > SoC.
> 
> I agree!
> 
> > 
> > > I think generic power-sequences
> > >   were the topic back then, though that might have changed over time?
> > > - There are places that describe "fixed-clock" as
> > >   "basic fixed-rate clock that cannot gate" [1]
> > 
> > I think that that is something that could be changed, it's "just" a
> > comment in some code! Sounds like Stephen disagrees though :)
> 
> It's more about making a clear break from the fixed-clock binding so
> that the extra properties are required.
> 
> > 
> > > - Stephen also suggested a separate binding [2]
> > 
> > I liked your "gated-oscillator" suggestion in another reply, but
> > "gated-fixed-clock" might be a better "thematic" fit since this is a
> > special case of fixed-clocks?
> > 
> 
> It looks to me like we've arrived at the hardest problem in computer
> science, i.e. naming. Any of these names is fine. I'd look to see what
> those parts on mouser are called and use that to drive the compatible
> name decision if you can't decide. The description section in the
> binding could be verbose and link to some parts/pdfs if that helps too.
> In the past I've seen EEs call these things clock buffers. I'm not a
> classically trained EE myself but it usually helps to use similar names
> from the schematic in DT because DT authors are sorta translating
> schematics to DT.

TL;DR: I'm fine with both "gated-oscillator" or "gated-fixed-clock" .
Some tiny part in the back of my head wants to name things in the most
specific way aka "gated-oscillator", but I guess "gated-fixed-clock" will
possibly spare us the naming dance in the future :-)

So I guess if nobody objects anymore, I'll go with "gated-fixed-clock".

--------- 8< --------
Some background stuff for the oscillator / clock-buffer difference,
which are actually both used on the Rock 5 ITX in question:

[my ASCII-art may not survive mail readers]
                                        ------------
VCC3v3_PI6C (to both VDD + Enable) -----| VCC*     | - CLKoutA - to PCIe
  |                                     |          |
--------------------                    |          | - CLKoutB - to PCIe
| 100MHz,3.3V,3225 |-------XTAL_IN_OUT -| Au5426   |
--------------------                    |          | - REFout (unconnected)
                                        ------------

Just asking Google for that "100MHz,3.3V,3225" brings me to
"100 MHz Standard Clock Oscillators" on Mouser.

The Au5426 from Aurasemi is a "4 Differential and 1 LVCMOS Output Ultra
Low Jitter High Performance Buffer" - aka a clock-buffer.

In the Rock-5-ITX patch, I opted to ignore it, because on _that_ board
it is transparent to the system, enabled by the same sources as the
crystal and statically configured.

On the other hand, the Au5426 actually _has_ input pins to select
its working mode: 
- select between different clock sources
- enable/disable the output of the input clock as refclk
- configure the clock buffer type (lvpecl, lvds, hcsl, hiz)

So I didn't want to conjure a binding for that stuff out of thin air :-)


Heiko

Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Dragan Simic]

Hello Stephen,

On 2024-07-27 00:21, Stephen Boyd wrote:
> Quoting Conor Dooley (2024-07-18 08:59:50)
>> 
>> FWIW, I wouldn't classify this as device-specific. "enable-gpios" and
>> "vdd-supply" are pretty generic and I think the latter is missing from
>> the vast majority of real* "fixed-clocks". I would expect that devices
>> where the datasheet would call
>> 
>> * Real because there's plenty of "fixed-clocks" (both in and out of 
>> tree)
>> that are used to work around the lack of a clock-controller driver for 
>> an
>> SoC.
> 
> I agree!
> 
>> > I think generic power-sequences
>> >   were the topic back then, though that might have changed over time?
>> > - There are places that describe "fixed-clock" as
>> >   "basic fixed-rate clock that cannot gate" [1]
>> 
>> I think that that is something that could be changed, it's "just" a
>> comment in some code! Sounds like Stephen disagrees though :)
> 
> It's more about making a clear break from the fixed-clock binding so
> that the extra properties are required.
>> 
>> > - Stephen also suggested a separate binding [2]
>> 
>> I liked your "gated-oscillator" suggestion in another reply, but
>> "gated-fixed-clock" might be a better "thematic" fit since this is a
>> special case of fixed-clocks?
> 
> It looks to me like we've arrived at the hardest problem in computer
> science, i.e. naming. Any of these names is fine. I'd look to see what
> those parts on mouser are called and use that to drive the compatible
> name decision if you can't decide. The description section in the
> binding could be verbose and link to some parts/pdfs if that helps too.
> In the past I've seen EEs call these things clock buffers. I'm not a
> classically trained EE myself but it usually helps to use similar names
> from the schematic in DT because DT authors are sorta translating
> schematics to DT.

Please note that the hardware components modeled by these DT bindings
are definitely not clock buffers.  Those are simply clock generators,
which can be gated, hense the proposal to use "gated-fixed-clock".

If we were to additionally model Aurasemi AU5426 or Diodes PI6C557 into
new DT bindings and into the board dts file(s), which I'd support 
because
of accuracy, those should actually be called "clock-buffer".