[PATCH v2 0/4] clk: Add basic register clock controller

[PATCH v2 0/4] clk: Add basic register clock controller

[David Yang]

These clocks were provided in `include/linux/clk-provider.h`, but lacks DT
bindings. Add a clock controller to avoid operation conflict on same
register.

v2: split clock controller and its clocks

Links:
v1: https://lore.kernel.org/r/20230414181302.986271-1-mmyangfl@gmail.com

David Yang (4):
  dt-bindings: clock: Add simple-clock-controller
  clk: Add simple clock controller
  dt-bindings: clock: Add gate-clock
  clk: gate: Add DT binding

 .../devicetree/bindings/clock/gate-clock.yaml |  58 ++++
 .../clock/simple-clock-controller.yaml        |  50 +++
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-gate.c                        |  81 +++++
 drivers/clk/clk-of.c                          | 292 ++++++++++++++++++
 drivers/clk/clk-of.h                          |  26 ++
 6 files changed, 508 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
 create mode 100644 drivers/clk/clk-of.c
 create mode 100644 drivers/clk/clk-of.h


base-commit: 7a934f4bd7d6f9da84c8812da3ba42ee10f5778e
-- 
2.39.2

[PATCH v2 1/4] dt-bindings: clock: Add simple-clock-controller

[David Yang]

Add DT bindings documentation for simple-clock-controller, mutex
controller for clocks.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 .../clock/simple-clock-controller.yaml        | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml

diff --git a/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
new file mode 100644
index 000000000000..17835aeddb1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/simple-clock-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Simple clock controller
+
+maintainers:
+  - David Yang <mmyangfl@gmail.com>
+
+description: |
+  Driver (lock provider) for real clocks.
+
+  Usually one register controls more than one clocks. This controller avoids
+  write conflicts by imposing a write lock, so that two operations on the same
+  register will not happen at the same time.
+
+properties:
+  compatible:
+    items:
+      - oneOf:
+          - const: simple-clock-controller
+          - const: simple-clock-reset-controller
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  '#reset-cells':
+    const: 2
+
+patternProperties:
+  "clock@.*":
+    type: object
+    description: Clock nodes.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@ffff000 {
+      compatible = "simple-clock-controller", "syscon", "simple-mfd";
+      reg = <0xffff000 0x1000>;
+    };
-- 
2.39.2

[PATCH v2 2/4] clk: Add simple clock controller

[David Yang]

These clocks were provided in `include/linux/clk-provider.h`, but lacks DT
bindings. Add a clock controller to avoid operation conflict on same
register.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/clk/Makefile |   1 +
 drivers/clk/clk-of.c | 292 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-of.h |  26 ++++
 3 files changed, 319 insertions(+)
 create mode 100644 drivers/clk/clk-of.c
 create mode 100644 drivers/clk/clk-of.h

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058a25..6cf0a888b673 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fractional-divider.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gpio.o
 ifeq ($(CONFIG_OF), y)
 obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-of.o
 endif
 
 # hardware specific clock types
diff --git a/drivers/clk/clk-of.c b/drivers/clk/clk-of.c
new file mode 100644
index 000000000000..3518ae848ed0
--- /dev/null
+++ b/drivers/clk/clk-of.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
+/*
+ * Copyright (c) 2023 David Yang
+ *
+ * Simple straight-forward register clocks bindings
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+
+#include "clk-of.h"
+
+struct of_clk_ctrl_priv {
+	spinlock_t lock;
+
+	struct reset_controller_dev rcdev;
+	void __iomem *base;
+	bool rst_set_to_disable;
+};
+
+static const struct of_clk_flag of_clk_common_flags[] = {
+	{ "set-rate-gate", CLK_SET_RATE_GATE },
+	{ "set-parent-gate", CLK_SET_PARENT_GATE },
+	{ "set-rate-parent", CLK_SET_RATE_PARENT },
+	{ "ignore-unused", CLK_IGNORE_UNUSED },
+	{ "get-rate-nocache", CLK_GET_RATE_NOCACHE },
+	{ "set-rate-no-reparent", CLK_SET_RATE_NO_REPARENT },
+	{ "get-accuracy-nocache", CLK_GET_ACCURACY_NOCACHE },
+	{ "recalc-new-rates", CLK_RECALC_NEW_RATES },
+	{ "set-rate-ungate", CLK_SET_RATE_UNGATE },
+	{ "critical", CLK_IS_CRITICAL },
+	{ "ops-parent-enable", CLK_OPS_PARENT_ENABLE },
+	{ "duty-cycle-parent", CLK_DUTY_CYCLE_PARENT },
+	{ }
+};
+
+void __iomem *of_clk_get_reg(struct device_node *np)
+{
+	u32 offset;
+	void __iomem *reg;
+
+	if (of_property_read_u32(np, "offset", &offset))
+		return NULL;
+
+	reg = of_iomap(np->parent, 0);
+	if (!reg)
+		return NULL;
+
+	return reg + offset;
+}
+EXPORT_SYMBOL_GPL(of_clk_get_reg);
+
+const char *of_clk_get_name(struct device_node *np)
+{
+	const char *name;
+
+	if (!of_property_read_string(np, "clock-output-name", &name))
+		return name;
+
+	return of_node_full_name(np);
+}
+EXPORT_SYMBOL_GPL(of_clk_get_name);
+
+unsigned long
+of_clk_get_flags(struct device_node *np, const struct of_clk_flag *defs)
+{
+	unsigned long flags = 0;
+
+	if (!defs)
+		defs = of_clk_common_flags;
+
+	for (int i = 0; defs[i].prop; i++)
+		if (of_property_read_bool(np, defs[i].prop))
+			flags |= defs[i].flag;
+
+	return flags;
+}
+EXPORT_SYMBOL_GPL(of_clk_get_flags);
+
+int of_clk_remove(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	of_clk_del_provider(np);
+	clk_hw_unregister(np->data);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_clk_remove);
+
+int of_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	int (*setup)(struct device_node *np) = of_device_get_match_data(dev);
+
+	return setup(np);
+}
+EXPORT_SYMBOL_GPL(of_clk_probe);
+
+/** of_rst_ctrl **/
+
+#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
+static int
+of_rst_ctrl_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct of_clk_ctrl_priv *priv = container_of(rcdev, struct of_clk_ctrl_priv, rcdev);
+	unsigned long flags;
+	u32 offset = id >> 16;
+	u8 index = id & 0x1f;
+	u32 val;
+
+	if (WARN_ON(!priv->base))
+		return 0;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	val = readl(priv->base + offset);
+	if (priv->rst_set_to_disable)
+		val &= ~BIT(index);
+	else
+		val |= BIT(index);
+	writel(val, priv->base + offset);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
+static int
+of_rst_ctrl_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct of_clk_ctrl_priv *priv = container_of(rcdev, struct of_clk_ctrl_priv, rcdev);
+	unsigned long flags;
+	u32 offset = id >> 16;
+	u8 index = id & 0x1f;
+	u32 val;
+
+	if (WARN_ON(!priv->base))
+		return 0;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	val = readl(priv->base + offset);
+	if (priv->rst_set_to_disable)
+		val |= BIT(index);
+	else
+		val &= ~BIT(index);
+	writel(val, priv->base + offset);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
+static const struct reset_control_ops of_rst_ctrl_ops = {
+	.assert = of_rst_ctrl_assert,
+	.deassert = of_rst_ctrl_deassert,
+};
+
+static int of_rst_ctrl_of_xlate(struct reset_controller_dev *rcdev,
+				 const struct of_phandle_args *reset_spec)
+{
+	return (reset_spec->args[0] << 16) | (reset_spec->args[1] & 0x1f);
+}
+
+static void of_rst_ctrl_unsetup(struct device_node *np)
+{
+	struct of_clk_ctrl_priv *priv = np->data;
+
+	reset_controller_unregister(&priv->rcdev);
+}
+
+static int of_rst_ctrl_setup(struct device_node *np, struct of_clk_ctrl_priv *priv)
+{
+	priv->base = of_iomap(np, 0);
+	priv->rst_set_to_disable = of_property_read_bool(np, "set-to-disable");
+
+	/* register no matter whether reg exists, to detect dts bug */
+	priv->rcdev.ops = &of_rst_ctrl_ops;
+	priv->rcdev.of_node = np;
+	priv->rcdev.of_reset_n_cells = 2;
+	priv->rcdev.of_xlate = of_rst_ctrl_of_xlate;
+	return reset_controller_register(&priv->rcdev);
+}
+#else
+static void of_rst_ctrl_unsetup(struct device_node *np)
+{
+}
+
+static int of_rst_ctrl_setup(struct device_node *np, struct of_clk_ctrl_priv *priv)
+{
+	return 0;
+}
+#endif
+
+/** of_crg_ctrl **/
+
+static void of_crg_ctrl_unsetup(struct device_node *np, bool crg)
+{
+	if (crg)
+		of_rst_ctrl_unsetup(np);
+
+	kfree(np->data);
+	np->data = NULL;
+}
+
+static int of_crg_ctrl_setup(struct device_node *np, bool crg)
+{
+	struct of_clk_ctrl_priv *priv;
+	int ret;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	np->data = priv;
+
+	spin_lock_init(&priv->lock);
+
+	if (crg) {
+		ret = of_rst_ctrl_setup(np, priv);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	kfree(np->data);
+	np->data = NULL;
+	return ret;
+}
+
+/** driver **/
+
+static void __init of_clk_ctrl_init(struct device_node *np)
+{
+	of_crg_ctrl_setup(np, false);
+}
+CLK_OF_DECLARE(of_clk_ctrl, "simple-clock-controller", of_clk_ctrl_init);
+
+static void __init of_crg_ctrl_init(struct device_node *np)
+{
+	of_crg_ctrl_setup(np, true);
+}
+CLK_OF_DECLARE(of_crg_ctrl, "simple-clock-reset-controller", of_crg_ctrl_init);
+
+static int of_crg_ctrl_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	bool crg = (bool) of_device_get_match_data(dev);
+
+	of_crg_ctrl_unsetup(np, crg);
+
+	return 0;
+}
+
+/* This function is not executed when of_clk_ctrl_init succeeded. */
+static int of_crg_ctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	bool crg = (bool) of_device_get_match_data(dev);
+
+	return of_crg_ctrl_setup(np, crg);
+}
+
+static const struct of_device_id of_crg_ctrl_ids[] = {
+	{ .compatible = "simple-clock-controller", .data = (void *) false },
+	{ .compatible = "simple-clock-reset-controller", .data = (void *) true },
+	{ }
+};
+
+static struct platform_driver of_crg_ctrl_driver = {
+	.driver = {
+		.name = "clk_of",
+		.of_match_table = of_crg_ctrl_ids,
+	},
+	.probe = of_crg_ctrl_probe,
+	.remove = of_crg_ctrl_remove,
+};
+builtin_platform_driver(of_crg_ctrl_driver);
diff --git a/drivers/clk/clk-of.h b/drivers/clk/clk-of.h
new file mode 100644
index 000000000000..ddb1e57ec2f1
--- /dev/null
+++ b/drivers/clk/clk-of.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later OR MIT */
+/*
+ * Copyright (c) 2023 David Yang
+ */
+
+#include <linux/spinlock_types.h>
+
+struct device_node;
+struct platform_device;
+
+struct of_clk_ctrl {
+	spinlock_t lock;
+};
+
+struct of_clk_flag {
+	const char *prop;
+	unsigned long flag;
+};
+
+void __iomem *of_clk_get_reg(struct device_node *np);
+const char *of_clk_get_name(struct device_node *np);
+unsigned long
+of_clk_get_flags(struct device_node *np, const struct of_clk_flag *defs);
+
+int of_clk_remove(struct platform_device *pdev);
+int of_clk_probe(struct platform_device *pdev);
-- 
2.39.2

[PATCH v2 3/4] dt-bindings: clock: Add gate-clock

[David Yang]

Add DT bindings documentation for gate-clock, which can gate its output.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 .../devicetree/bindings/clock/gate-clock.yaml | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/gate-clock.yaml b/Documentation/devicetree/bindings/clock/gate-clock.yaml
new file mode 100644
index 000000000000..3c993cb7e9bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gate-clock.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/gate-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clock which can gate its output
+
+maintainers:
+  - David Yang <mmyangfl@gmail.com>
+
+description: |
+  Clock which can gate its output.
+
+  The registers map is retrieved from the parental dt-node. So the clock node
+  should be represented as a sub-node of a "clock-controller" node.
+
+  See also: linux/clk-provider.h
+
+properties:
+  compatible:
+    const: gate-clock
+
+  '#clock-cells':
+    const: 0
+
+  clocks:
+    maxItems: 1
+    description: Parent clock.
+
+  offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset in the register map for the control register (in bytes).
+
+  bits:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Bit index which controls the output.
+
+  clock-output-names:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#clock-cells'
+  - offset
+  - bits
+
+additionalProperties: false
+
+examples:
+  - |
+    gate-clock@cc.3 {
+      compatible = "gate-clock";
+      #clock-cells = <0>;
+      offset = <0xcc>;
+      bits = <3>;
+      clock-output-names = "my-clk";
+    };
-- 
2.39.2

[PATCH v2 4/4] clk: gate: Add DT binding

[David Yang]

Add DT binding for gate clock as "gate-clock".

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/clk/clk-gate.c | 81 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 64283807600b..a70df4a2a9a7 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -12,8 +12,11 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/err.h>
+#include <linux/platform_device.h>
 #include <linux/string.h>
 
+#include "clk-of.h"
+
 /**
  * DOC: basic gatable clock which can gate and ungate it's ouput
  *
@@ -257,3 +260,81 @@ struct clk_hw *__devm_clk_hw_register_gate(struct device *dev,
 	return hw;
 }
 EXPORT_SYMBOL_GPL(__devm_clk_hw_register_gate);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_clk_flag of_clk_gate_flags[] = {
+	{ "set-to-disable", CLK_GATE_SET_TO_DISABLE },
+	{ "hiword-mask", CLK_GATE_HIWORD_MASK },
+	{ "big-endian", CLK_GATE_BIG_ENDIAN },
+	{ }
+};
+
+static int of_clk_gate_setup(struct device_node *np)
+{
+	struct of_clk_ctrl *ctrl = np->parent->data;
+	const char *name;
+	void __iomem *reg;
+	u32 bit_idx;
+
+	const char *property;
+	struct clk_hw *hw;
+	int ret;
+
+	reg = of_clk_get_reg(np);
+	if (!reg)
+		return -ENOMEM;
+	name = of_clk_get_name(np);
+	if (!name)
+		return -EINVAL;
+
+	property = "bits";
+	if (of_property_read_u32(np, property, &bit_idx))
+		goto err_property;
+
+	hw = __clk_hw_register_gate(NULL, np, name,
+				    of_clk_get_parent_name(np, 0),
+				    NULL, NULL, of_clk_get_flags(np, NULL),
+				    reg, bit_idx,
+				    of_clk_get_flags(np, of_clk_gate_flags),
+				    &ctrl->lock);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
+	if (ret)
+		goto err_register;
+
+	np->data = hw;
+	return 0;
+
+err_register:
+	clk_hw_unregister(hw);
+	return ret;
+
+err_property:
+	pr_err("%s: clock %s missing required property \"%s\"\n",
+	       __func__, name, property);
+	return -EINVAL;
+}
+
+static void __init of_clk_gate_init(struct device_node *np)
+{
+	of_clk_gate_setup(np);
+}
+CLK_OF_DECLARE(of_clk_gate, "gate-clock", of_clk_gate_init);
+
+static const struct of_device_id of_clk_gate_ids[] = {
+	{ .compatible = "gate-clock", .data = of_clk_gate_setup },
+	{ }
+};
+
+static struct platform_driver of_clk_gate_driver = {
+	.driver = {
+		.name = "clk_gate",
+		.of_match_table = of_clk_gate_ids,
+	},
+	.probe = of_clk_probe,
+	.remove = of_clk_remove,
+};
+builtin_platform_driver(of_clk_gate_driver);
+#endif
-- 
2.39.2

Re: [PATCH v2 1/4] dt-bindings: clock: Add simple-clock-controller

[Krzysztof Kozlowski]

On 16/04/2023 19:32, David Yang wrote:
> Add DT bindings documentation for simple-clock-controller, mutex
> controller for clocks.
> 
> Signed-off-by: David Yang <mmyangfl@gmail.com>
> ---
>  .../clock/simple-clock-controller.yaml        | 50 +++++++++++++++++++

Where is the changelog?

>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> new file mode 100644
> index 000000000000..17835aeddb1d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/simple-clock-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Simple clock controller
> +
> +maintainers:
> +  - David Yang <mmyangfl@gmail.com>
> +
> +description: |
> +  Driver (lock provider) for real clocks.

Drop driver references. Typo: clock, not lock.

What is a real clock? What is an unreal clock?

> +
> +  Usually one register controls more than one clocks. This controller avoids
> +  write conflicts by imposing a write lock, so that two operations on the same
> +  register will not happen at the same time.

Interesting. How the clock controller imposes write locks? Aren't you
now mixing drivers and hardware?


> +
> +properties:
> +  compatible:
> +    items:
> +      - oneOf:
> +          - const: simple-clock-controller
> +          - const: simple-clock-reset-controller

Why two?

> +      - const: syscon
> +      - const: simple-mfd

Why do you need syscon and simple-mfd?

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#reset-cells':
> +    const: 2
> +
> +patternProperties:
> +  "clock@.*":

Use consistent quotes.

Anyway, I don't understand what is happening here and why such changes.
Nothing is explained...


Best regards,
Krzysztof

Re: [PATCH v2 3/4] dt-bindings: clock: Add gate-clock

[Krzysztof Kozlowski]

On 16/04/2023 19:33, David Yang wrote:
> Add DT bindings documentation for gate-clock, which can gate its output.
> 
> Signed-off-by: David Yang <mmyangfl@gmail.com>
> ---
>  .../devicetree/bindings/clock/gate-clock.yaml | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.yaml

Where is the changelog? What happened here?

> 
> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.yaml b/Documentation/devicetree/bindings/clock/gate-clock.yaml
> new file mode 100644
> index 000000000000..3c993cb7e9bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gate-clock.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/gate-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock which can gate its output
> +
> +maintainers:
> +  - David Yang <mmyangfl@gmail.com>
> +
> +description: |
> +  Clock which can gate its output.
> +
> +  The registers map is retrieved from the parental dt-node. So the clock node
> +  should be represented as a sub-node of a "clock-controller" node.

If this is supposed to be used in parent schema, then reference it there.

> +
> +  See also: linux/clk-provider.h

How is this related to hardware? Also, referencing linux headers is
usually not good idea for bindings.
> +
> +properties:
> +  compatible:
> +    const: gate-clock
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  clocks:
> +    maxItems: 1
> +    description: Parent clock.
> +
> +  offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset in the register map for the control register (in bytes).
> +
> +  bits:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Bit index which controls the output.
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - offset
> +  - bits
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gate-clock@cc.3 {

So you keep ignoring the comments... I don't know what happened here but
this code for sure looks wrong.

Did you test the changes?

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: clock: Add simple-clock-controller

[Yangfl]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年4月17日周一 01:38写道：
>
> On 16/04/2023 19:32, David Yang wrote:
> > Add DT bindings documentation for simple-clock-controller, mutex
> > controller for clocks.
> >
> > Signed-off-by: David Yang <mmyangfl@gmail.com>
> > ---
> >  .../clock/simple-clock-controller.yaml        | 50 +++++++++++++++++++
>
> Where is the changelog?

What changelog? Series changelog already included in series cover.

Re: [PATCH v2 1/4] dt-bindings: clock: Add simple-clock-controller

[Krzysztof Kozlowski]

On 16/04/2023 20:22, Yangfl wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年4月17日周一 01:38写道：
>>
>> On 16/04/2023 19:32, David Yang wrote:
>>> Add DT bindings documentation for simple-clock-controller, mutex
>>> controller for clocks.
>>>
>>> Signed-off-by: David Yang <mmyangfl@gmail.com>
>>> ---
>>>  .../clock/simple-clock-controller.yaml        | 50 +++++++++++++++++++
>>
>> Where is the changelog?
> 
> What changelog? Series changelog already included in series cover.

Changelog of each patch, since there is no cover letter. If you skip
some people from CC, then be sure they get cover letter (if such exists).


Best regards,
Krzysztof

Re: [PATCH v2 3/4] dt-bindings: clock: Add gate-clock

[Yangfl]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年4月17日周一 01:41写道：
>
> Did you test the changes?

I don't know what "test" is here. make dt_binding_check passed and it
can work with the driver.

Re: [PATCH v2 3/4] dt-bindings: clock: Add gate-clock

[Krzysztof Kozlowski]

On 16/04/2023 20:28, Yangfl wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年4月17日周一 01:41写道：
>>
>> Did you test the changes?
> 
> I don't know what "test" is here. make dt_binding_check passed and it
> can work with the driver.

I don't think so. If you tested it, for sure you would see warnings like
"node has a unit name, but no reg or ranges property".

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: clock: Add simple-clock-controller

[Yangfl]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年4月17日周一 01:38写道：
>
> On 16/04/2023 19:32, David Yang wrote:
> > Add DT bindings documentation for simple-clock-controller, mutex
> > controller for clocks.
> >
> > Signed-off-by: David Yang <mmyangfl@gmail.com>
> > ---
> >  .../clock/simple-clock-controller.yaml        | 50 +++++++++++++++++++
>
> Where is the changelog?
>

Cover now send with v3.

> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> > new file mode 100644
> > index 000000000000..17835aeddb1d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/simple-clock-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Simple clock controller
> > +
> > +maintainers:
> > +  - David Yang <mmyangfl@gmail.com>
> > +
> > +description: |
> > +  Driver (lock provider) for real clocks.
>
> Drop driver references. Typo: clock, not lock.
>
> What is a real clock? What is an unreal clock?
>

Rewrite description in v3. The controller is kinda unreal since it
does not require any operation to "enable" the controller, but such
description is avoided in v3.

> > +
> > +  Usually one register controls more than one clocks. This controller avoids
> > +  write conflicts by imposing a write lock, so that two operations on the same
> > +  register will not happen at the same time.
>
> Interesting. How the clock controller imposes write locks? Aren't you
> now mixing drivers and hardware?

Avoided driver details in device description in v3.

>
>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - oneOf:
> > +          - const: simple-clock-controller
> > +          - const: simple-clock-reset-controller
>
> Why two?

It may also handle reset requests. But removed in v3 for further consideration.

>
> > +      - const: syscon
> > +      - const: simple-mfd
>
> Why do you need syscon and simple-mfd?

Kinda typo. Removed in v3.

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#reset-cells':
> > +    const: 2
> > +
> > +patternProperties:
> > +  "clock@.*":
>
> Use consistent quotes.

Fixed in v3.

>
> Anyway, I don't understand what is happening here and why such changes.
> Nothing is explained...
>
>
> Best regards,
> Krzysztof
>