[PATCH v3 0/5] Binding and driver for gated-fixed-clocks

[PATCH v3 0/5] Binding and driver for gated-fixed-clocks

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


With the discussion in v2, gated-fixed-clock was deemed one possible
nice naming, so I did go with that.
Stephen also suggested reusing more of clk-gpio to not re-implement the
gpio handling wrt. sleeping and non-sleeping gpios.

Though instead of exporting masses of structs and ops, gated-fixed-clock
is quite close to the other gpio-clocks, so I've put it into the clk-gpio
file.


changes in v3:
- rename to gated-fixed-clock (Conor)
- move into clk-gpio
- some tiny cleanups to the existing clk-gpio drivers

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 (5):
  dt-bindings: clocks: add binding for gated-fixed-clocks
  clk: clk-gpio: update documentation for gpio-gate clock
  clk: clk-gpio: use dev_err_probe for gpio-get failure
  clk: clk-gpio: add driver for gated-fixed-clocks
  arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

 .../bindings/clock/gated-fixed-clock.yaml     |  49 +++++
 .../boot/dts/rockchip/rk3588-rock-5-itx.dts   |  38 +++-
 drivers/clk/clk-gpio.c                        | 202 ++++++++++++++++--
 3 files changed, 273 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml

-- 
2.43.0

[PATCH v3 1/5] dt-bindings: clocks: add binding for gated-fixed-clocks

[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/gated-fixed-clock.yaml     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml b/Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml
new file mode 100644
index 000000000000..76d264770d35
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gated-fixed-clock.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/gated-fixed-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Gated Fixed clock
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+properties:
+  compatible:
+    const: gated-fixed-clock
+
+  "#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 = "gated-fixed-clock";
+      #clock-cells = <0>;
+      clock-frequency = <1000000000>;
+      vdd-supply = <&reg_vdd>;
+    };
+...
-- 
2.43.0

Re: [PATCH v3 1/5] dt-bindings: clocks: add binding for gated-fixed-clocks

[Rob Herring]

On Wed, Aug 28, 2024 at 12:14:59PM +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/gated-fixed-clock.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml b/Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml
> new file mode 100644
> index 000000000000..76d264770d35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gated-fixed-clock.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/gated-fixed-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Gated Fixed clock
> +
> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +properties:
> +  compatible:
> +    const: gated-fixed-clock
> +
> +  "#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 {

clock-1000000000

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


> +      compatible = "gated-fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency = <1000000000>;
> +      vdd-supply = <&reg_vdd>;
> +    };
> +...
> -- 
> 2.43.0
> 

Re: [PATCH v3 1/5] dt-bindings: clocks: add binding for gated-fixed-clocks

[Conor Dooley]

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

On Wed, Aug 28, 2024 at 12:14:59PM +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>

With Rob's fix,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Think the current name is more clear about what this is, thanks.

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

[PATCH v3 2/5] clk: clk-gpio: update documentation for gpio-gate clock

[Heiko Stuebner]

The main documentation block seems to be from a time before the driver
handled sleeping and non-sleeping gpios and which that change it seems
updating the doc was overlooked. So do that now.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 5b114043771d..98415782f9a2 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -22,8 +22,9 @@
  * DOC: basic gpio gated clock which can be enabled and disabled
  *      with gpio output
  * Traits of this clock:
- * prepare - clk_(un)prepare only ensures parent is (un)prepared
- * enable - clk_enable and clk_disable are functional & control gpio
+ * prepare - clk_(un)prepare are functional and control a gpio that can sleep
+ * enable - clk_enable and clk_disable are functional & control
+ *          non-sleeping gpio
  * rate - inherits rate from parent.  No clk_set_rate support
  * parent - fixed parent.  No clk_set_parent support
  */
-- 
2.43.0

Re: [PATCH v3 2/5] clk: clk-gpio: update documentation for gpio-gate clock

[Diederik de Haas]

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

On Wed Aug 28, 2024 at 12:15 PM CEST, Heiko Stuebner wrote:
> The main documentation block seems to be from a time before the driver
> handled sleeping and non-sleeping gpios and which that change it seems

s/which/with/ ?

> updating the doc was overlooked. So do that now.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>


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

[PATCH v3 3/5] clk: clk-gpio: use dev_err_probe for gpio-get failure

[Heiko Stuebner]

This is a real driver and dev_err_probe will hide the distinction between
EPROBE_DEFER and other errors automatically, so there is no need to
open-code this.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-gpio.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 98415782f9a2..cda362a2eca0 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -200,7 +200,6 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	struct gpio_desc *gpiod;
 	struct clk_hw *hw;
 	bool is_mux;
-	int ret;
 
 	is_mux = of_device_is_compatible(node, "gpio-mux-clock");
 
@@ -212,17 +211,9 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 
 	gpio_name = is_mux ? "select" : "enable";
 	gpiod = devm_gpiod_get(dev, gpio_name, GPIOD_OUT_LOW);
-	if (IS_ERR(gpiod)) {
-		ret = PTR_ERR(gpiod);
-		if (ret == -EPROBE_DEFER)
-			pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
-					node, __func__);
-		else
-			pr_err("%pOFn: %s: Can't get '%s' named GPIO property\n",
-					node, __func__,
-					gpio_name);
-		return ret;
-	}
+	if (IS_ERR(gpiod))
+		return dev_err_probe(dev, PTR_ERR(gpiod),
+				     "Can't get '%s' named GPIO property\n", gpio_name);
 
 	if (is_mux)
 		hw = clk_hw_register_gpio_mux(dev, gpiod);
-- 
2.43.0

[PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

[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 gated-fixed-clocks
that can show up in schematics looking like

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

The new driver gets grouped together with the existing gpio-gate and
gpio-mux, as it for one re-uses a lot of the gpio-gate functions
and also in it's core it's just another gpio-controlled clock, just
with a fixed rate and a regulator-supply added in.

The regulator-API provides function stubs for the !CONFIG_REGULATOR case,
so no special handling is necessary.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-gpio.c | 182 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index cda362a2eca0..8bcdef340b4c 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 
 /**
  * DOC: basic gpio gated clock which can be enabled and disabled
@@ -239,3 +240,184 @@ static struct platform_driver gpio_clk_driver = {
 	},
 };
 builtin_platform_driver(gpio_clk_driver);
+
+/**
+ * DOC: gated fixed clock, controlled with a gpio output and a regulator
+ * Traits of this clock:
+ * prepare - clk_prepare and clk_unprepare are function & control regulator
+ *           optionally a gpio that can sleep
+ * enable - clk_enable and clk_disable are functional & control gpio
+ * rate - rate is fixed and set on clock generation
+ * parent - fixed clock is a root clock and has no parent.
+ */
+
+/**
+ * struct clk_gate_fixed - gated-fixed-clock
+ *
+ * clk_gpio:	instance of clk_gpio for gate-gpio
+ * supply:	supply regulator
+ * rate:	fixed rate
+ */
+struct clk_gated_fixed {
+	struct clk_gpio clk_gpio;
+	struct regulator *supply;
+	u32 rate;
+};
+
+#define to_clk_gated_fixed(_clk_gpio) container_of(_clk_gpio, struct clk_gated_fixed, clk_gpio)
+
+static unsigned long clk_gated_fixed_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	return to_clk_gated_fixed(to_clk_gpio(hw))->rate;
+}
+
+static int clk_gated_fixed_prepare(struct clk_hw *hw)
+{
+	struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
+
+	if (!clk->supply)
+		return 0;
+
+	return regulator_enable(clk->supply);
+}
+
+static void clk_gated_fixed_unprepare(struct clk_hw *hw)
+{
+	struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
+
+	if (!clk->supply)
+		return;
+
+	regulator_disable(clk->supply);
+}
+
+static int clk_gated_fixed_is_prepared(struct clk_hw *hw)
+{
+	struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
+
+	if (!clk->supply)
+		return true;
+
+	return regulator_is_enabled(clk->supply);
+}
+
+/*
+ * Fixed gated clock with non-sleeping gpio.
+ *
+ * Prepare operation turns on the supply regulator
+ * and the enable operation switches the enable-gpio.
+ */
+const struct clk_ops clk_gated_fixed_ops = {
+	.prepare = clk_gated_fixed_prepare,
+	.unprepare = clk_gated_fixed_unprepare,
+	.is_prepared = clk_gated_fixed_is_prepared,
+	.enable = clk_gpio_gate_enable,
+	.disable = clk_gpio_gate_disable,
+	.is_enabled = clk_gpio_gate_is_enabled,
+	.recalc_rate = clk_gated_fixed_recalc_rate,
+};
+
+static int clk_sleeping_gated_fixed_prepare(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = clk_gated_fixed_prepare(hw);
+	if (ret)
+		return ret;
+
+	ret = clk_sleeping_gpio_gate_prepare(hw);
+	if (ret)
+		clk_gated_fixed_unprepare(hw);
+
+	return ret;
+}
+
+static void clk_sleeping_gated_fixed_unprepare(struct clk_hw *hw)
+{
+	clk_gated_fixed_unprepare(hw);
+	clk_sleeping_gpio_gate_unprepare(hw);
+}
+
+/*
+ * Fixed gated clock with non-sleeping gpio.
+ *
+ * Enabling the supply regulator and switching the enable-gpio happens
+ * both in the prepare step.
+ * is_prepared only needs to check the gpio state, as toggling the
+ * gpio is the last step when preparing.
+ */
+const struct clk_ops clk_sleeping_gated_fixed_ops = {
+	.prepare = clk_sleeping_gated_fixed_prepare,
+	.unprepare = clk_sleeping_gated_fixed_unprepare,
+	.is_prepared = clk_sleeping_gpio_gate_is_prepared,
+	.recalc_rate = clk_gated_fixed_recalc_rate,
+};
+
+static int clk_gated_fixed_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_gated_fixed *clk;
+	const struct clk_ops *ops;
+	const char *clk_name;
+	int ret;
+
+	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return -ENOMEM;
+
+	if (device_property_read_u32(dev, "clock-frequency", &clk->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);
+
+	clk->supply = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(clk->supply)) {
+		if (PTR_ERR(clk->supply) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(clk->supply),
+					     "failed to get regulator\n");
+		clk->supply = NULL;
+	}
+
+	clk->clk_gpio.gpiod = devm_gpiod_get_optional(dev, "enable",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(clk->clk_gpio.gpiod))
+		return dev_err_probe(dev, PTR_ERR(clk->clk_gpio.gpiod),
+				     "failed to get gpio\n");
+
+	if (gpiod_cansleep(clk->clk_gpio.gpiod))
+		ops = &clk_sleeping_gated_fixed_ops;
+	else
+		ops = &clk_gated_fixed_ops;
+
+	clk->clk_gpio.hw.init = CLK_HW_INIT_NO_PARENT(clk_name, ops, 0);
+
+	/* register the clock */
+	ret = devm_clk_hw_register(dev, &clk->clk_gpio.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,
+					  &clk->clk_gpio.hw);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register clock provider\n");
+
+	return 0;
+}
+
+static const struct of_device_id gated_fixed_clk_match_table[] = {
+	{ .compatible = "gated-fixed-clock" },
+};
+
+static struct platform_driver gated_fixed_clk_driver = {
+	.probe		= clk_gated_fixed_probe,
+	.driver		= {
+		.name	= "gated-fixed-clk",
+		.of_match_table = gated_fixed_clk_match_table,
+	},
+};
+builtin_platform_driver(gated_fixed_clk_driver);
-- 
2.43.0

Re: [PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

[Stephen Boyd]

Quoting Heiko Stuebner (2024-08-28 03:15:02)
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index cda362a2eca0..8bcdef340b4c 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -239,3 +240,184 @@ static struct platform_driver gpio_clk_driver = {
>         },
>  };
>  builtin_platform_driver(gpio_clk_driver);
> +
> +/**
> + * DOC: gated fixed clock, controlled with a gpio output and a regulator
> + * Traits of this clock:
> + * prepare - clk_prepare and clk_unprepare are function & control regulator
> + *           optionally a gpio that can sleep
> + * enable - clk_enable and clk_disable are functional & control gpio
> + * rate - rate is fixed and set on clock generation

Maybe 'clock registration'

> + * parent - fixed clock is a root clock and has no parent.

Not sure why this one gets the period while other lines above don't.

> + */
> +
> +/**
> + * struct clk_gate_fixed - gated-fixed-clock
> + *
> + * clk_gpio:   instance of clk_gpio for gate-gpio
> + * supply:     supply regulator
> + * rate:       fixed rate
> + */
> +struct clk_gated_fixed {
> +       struct clk_gpio clk_gpio;
> +       struct regulator *supply;
> +       u32 rate;

unsigned long rate to match the CCF type please.

> +};
> +
> +#define to_clk_gated_fixed(_clk_gpio) container_of(_clk_gpio, struct clk_gated_fixed, clk_gpio)
> +
> +static unsigned long clk_gated_fixed_recalc_rate(struct clk_hw *hw,
> +                                                unsigned long parent_rate)
> +{
> +       return to_clk_gated_fixed(to_clk_gpio(hw))->rate;
> +}
> +
> +static int clk_gated_fixed_prepare(struct clk_hw *hw)
> +{
> +       struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> +       if (!clk->supply)
> +               return 0;
> +
> +       return regulator_enable(clk->supply);
> +}
> +
> +static void clk_gated_fixed_unprepare(struct clk_hw *hw)
> +{
> +       struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> +       if (!clk->supply)
> +               return;
> +
> +       regulator_disable(clk->supply);
> +}
> +
> +static int clk_gated_fixed_is_prepared(struct clk_hw *hw)
> +{
> +       struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> +       if (!clk->supply)
> +               return true;
> +
> +       return regulator_is_enabled(clk->supply);
> +}
> +
> +/*
> + * Fixed gated clock with non-sleeping gpio.
> + *
> + * Prepare operation turns on the supply regulator
> + * and the enable operation switches the enable-gpio.
> + */
> +const struct clk_ops clk_gated_fixed_ops = {

static

> +       .prepare = clk_gated_fixed_prepare,
> +       .unprepare = clk_gated_fixed_unprepare,
> +       .is_prepared = clk_gated_fixed_is_prepared,
> +       .enable = clk_gpio_gate_enable,
> +       .disable = clk_gpio_gate_disable,
> +       .is_enabled = clk_gpio_gate_is_enabled,
> +       .recalc_rate = clk_gated_fixed_recalc_rate,
> +};
> +
> +static int clk_sleeping_gated_fixed_prepare(struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = clk_gated_fixed_prepare(hw);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_sleeping_gpio_gate_prepare(hw);
> +       if (ret)
> +               clk_gated_fixed_unprepare(hw);
> +
> +       return ret;
> +}
> +
> +static void clk_sleeping_gated_fixed_unprepare(struct clk_hw *hw)
> +{
> +       clk_gated_fixed_unprepare(hw);
> +       clk_sleeping_gpio_gate_unprepare(hw);
> +}
> +
> +/*
> + * Fixed gated clock with non-sleeping gpio.
> + *
> + * Enabling the supply regulator and switching the enable-gpio happens
> + * both in the prepare step.
> + * is_prepared only needs to check the gpio state, as toggling the
> + * gpio is the last step when preparing.
> + */
> +const struct clk_ops clk_sleeping_gated_fixed_ops = {

static

> +       .prepare = clk_sleeping_gated_fixed_prepare,
> +       .unprepare = clk_sleeping_gated_fixed_unprepare,
> +       .is_prepared = clk_sleeping_gpio_gate_is_prepared,
> +       .recalc_rate = clk_gated_fixed_recalc_rate,
> +};
> +
> +static int clk_gated_fixed_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct clk_gated_fixed *clk;
> +       const struct clk_ops *ops;
> +       const char *clk_name;
> +       int ret;
> +
> +       clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return -ENOMEM;
> +
> +       if (device_property_read_u32(dev, "clock-frequency", &clk->rate))

Why not return the error code?

> +               return dev_err_probe(dev, -EIO, "failed to get clock-frequency");

Missing newline on printk.

> +
> +       ret = device_property_read_string(dev, "clock-output-names", &clk_name);
> +       if (ret)
> +               clk_name = fwnode_get_name(dev->fwnode);
> +
> +       clk->supply = devm_regulator_get_optional(dev, "vdd");
> +       if (IS_ERR(clk->supply)) {
> +               if (PTR_ERR(clk->supply) != -ENODEV)
> +                       return dev_err_probe(dev, PTR_ERR(clk->supply),
> +                                            "failed to get regulator\n");
> +               clk->supply = NULL;
> +       }
> +
> +       clk->clk_gpio.gpiod = devm_gpiod_get_optional(dev, "enable",
> +                                                     GPIOD_OUT_LOW);
> +       if (IS_ERR(clk->clk_gpio.gpiod))
> +               return dev_err_probe(dev, PTR_ERR(clk->clk_gpio.gpiod),
> +                                    "failed to get gpio\n");
> +
> +       if (gpiod_cansleep(clk->clk_gpio.gpiod))
> +               ops = &clk_sleeping_gated_fixed_ops;
> +       else
> +               ops = &clk_gated_fixed_ops;
> +
> +       clk->clk_gpio.hw.init = CLK_HW_INIT_NO_PARENT(clk_name, ops, 0);
> +
> +       /* register the clock */
> +       ret = devm_clk_hw_register(dev, &clk->clk_gpio.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,
> +                                         &clk->clk_gpio.hw);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "failed to register clock provider\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id gated_fixed_clk_match_table[] = {
> +       { .compatible = "gated-fixed-clock" },

Add a sentinel.

> +};
> +
> +static struct platform_driver gated_fixed_clk_driver = {
> +       .probe          = clk_gated_fixed_probe,
> +       .driver         = {
> +               .name   = "gated-fixed-clk",
> +               .of_match_table = gated_fixed_clk_match_table,
> +       },
> +};
> +builtin_platform_driver(gated_fixed_clk_driver);

The comment above builtin_platform_driver says "Each driver may only use
this macro once". Seems that we need to expand the macro.

Re: [PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

[Heiko Stübner]

Am Mittwoch, 28. August 2024, 20:30:51 CEST schrieb Stephen Boyd:
> Quoting Heiko Stuebner (2024-08-28 03:15:02)

[leaving out all the "will fix" parts :-) ]

> > +static struct platform_driver gated_fixed_clk_driver = {
> > +       .probe          = clk_gated_fixed_probe,
> > +       .driver         = {
> > +               .name   = "gated-fixed-clk",
> > +               .of_match_table = gated_fixed_clk_match_table,
> > +       },
> > +};
> > +builtin_platform_driver(gated_fixed_clk_driver);
> 
> The comment above builtin_platform_driver says "Each driver may only use
> this macro once". Seems that we need to expand the macro.

each _driver_, not each file is the important point I think.

Looking at the code generation, it just wants to use the name of the
driver struct for generating the init functions.

So in the builtin_driver macro [0] it wants to use the
gated_fixed_clk_driver to create the init-function as
gated_fixed_clk_driver_init() hence anybody using the macro a second time
for the same driver would create that function two times.

Also as can be seen with the imx gpc driver [1], the two-drivers in the
same file is already in use.

I've also done a practical test with that and did [2], which resulted in
both drivers getting registered as expected:
[    0.132087] ----init gpio_clk_driver
[    0.132160] ----init gated_fixed_clk_driver


So not sure, if I misinterpreted your comment, but I don't think changes
are necessary for this portion.

Heiko


[0] https://elixir.bootlin.com/linux/v6.10.8/source/include/linux/device/driver.h#L284
[1]
https://elixir.bootlin.com/linux/v6.10.8/source/drivers/pmdomain/imx/gpc.c#L239
https://elixir.bootlin.com/linux/v6.10.8/source/drivers/pmdomain/imx/gpc.c#L556

[2]
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 1fc8b68786de..e306f554cd0f 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -284,6 +284,7 @@ module_exit(__driver##_exit);
 #define builtin_driver(__driver, __register, ...) \
 static int __init __driver##_init(void) \
 { \
+       printk("----init %s\n", __stringify(__driver)); \
        return __register(&(__driver) , ##__VA_ARGS__); \
 } \
 device_initcall(__driver##_init);

Re: [PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

[Stephen Boyd]

Quoting Heiko Stübner (2024-09-05 15:48:35)
> Am Mittwoch, 28. August 2024, 20:30:51 CEST schrieb Stephen Boyd:
> > Quoting Heiko Stuebner (2024-08-28 03:15:02)
> 
> [leaving out all the "will fix" parts :-) ]
> 
> > > +static struct platform_driver gated_fixed_clk_driver = {
> > > +       .probe          = clk_gated_fixed_probe,
> > > +       .driver         = {
> > > +               .name   = "gated-fixed-clk",
> > > +               .of_match_table = gated_fixed_clk_match_table,
> > > +       },
> > > +};
> > > +builtin_platform_driver(gated_fixed_clk_driver);
> > 
> > The comment above builtin_platform_driver says "Each driver may only use
> > this macro once". Seems that we need to expand the macro.
> 
> each _driver_, not each file is the important point I think.

Ok!

[PATCH v3 5/5] 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 d0b922b8d67e..2d0bcf90bf0f 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 gated oscillator: 100MHz,3.3V,3225 */
+	pcie30_port0_refclk: pcie30_port1_refclk: pcie-oscillator {
+		compatible = "gated-fixed-clock";
+		#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.43.0