Skip to contentSkip to conversation

clk: Add basic register clock controller

PATCH NAK David Yang 5 participants 12 messages Apr 16, 2023 – Apr 18, 2023

Subsystems (2)
lore
Share & tools
lore

David Yang

author
#0 lore raw

v3 cover clk: Add basic register clock controller

These clocks were provided in include/linux/clk-provider.h, but lacks DT bindings. Add DT bindings for them.

v2: split clock controller and its clocks
v3:
* rewrite commit message
* rewrite descriptions in dt-bindings to describe the devices
* fix format error of dt-bindings

Links:
v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]

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 |  59 ++++
 .../clock/simple-clock-controller.yaml        |  39 +++
 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, 498 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

Signature · 1 line
2.39.2
author
#1 lore raw

PATCH v3 1/4 dt-bindings: clock: Add simple-clock-controller

Add DT bindings documentation for simple clock controller, just a contiguous region of multiple clock registers.

---
 .../clock/simple-clock-controller.yaml        | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
Documentation/devicetree/bindings/clock/simple-clock-controller.yaml new file +39 -0
new file mode 100644
index 000000000000..3d9b436b0ef9
@@ -0,0 +1,39 @@
+# 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 <[email protected]>
+
+description: |
+  A contiguous register region of multiple different clocks. No operations are
+  required to enable or disable the clock controller.
+
+properties:
+  compatible:
+    const: simple-clock-controller
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  '.*clock.*':
+    type: object
+    description: Clock devices.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@ffff000 {
+      compatible = "simple-clock-controller";
+      reg = <0xffff000 0x1000>;
+    };
Signature · 1 line
2.39.2
#2 lore raw

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

Quoted 31 lines · David Yang

Add DT bindings documentation for simple clock controller, just a contiguous region of multiple clock registers.

---
 .../clock/simple-clock-controller.yaml        | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
Documentation/devicetree/bindings/clock/simple-clock-controller.yaml new file +16 -0
new file mode 100644
index 000000000000..3d9b436b0ef9
@@ -0,0 +1,39 @@
+# 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 <[email protected]>
+
+description: |
+  A contiguous register region of multiple different clocks. No operations are
+  required to enable or disable the clock controller.
+
+properties:

Documentation/devicetree/bindings/clock/simple-clock-controller.yaml line 26

Quoted 10 lines
Documentation/devicetree/bindings/clock/simple-clock-controller.yaml new file +10 -0
+  compatible:
+    const: simple-clock-controller
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  '.*clock.*':
+    type: object
+    description: Clock devices.

I don't know what improved here... this is still without any refs.

It looks like you ignored my feedback from v1 and maybe v2, so respond to each of it.

Best regards,
Krzysztof

#3 lore raw

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

In reply to David Yang
Quoted 6 lines · David Yang
Documentation/devicetree/bindings/clock/simple-clock-controller.yaml new file +6 -0
+patternProperties:
+  '.*clock.*':
+    type: object
+    description: Clock devices.
+
+required:

Documentation/devicetree/bindings/clock/simple-clock-controller.yaml line 38

Quoted 10 lines
Documentation/devicetree/bindings/clock/simple-clock-controller.yaml new file +10 -0
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@ffff000 {
+      compatible = "simple-clock-controller";
+      reg = <0xffff000 0x1000>;

BTW, this is just incomplete example. Fill it with proper children.

Best regards,
Krzysztof

author
#4 lore raw

PATCH v3 2/4 clk: Add simple clock controller

In reply to David Yang

Basic common clocks were provided in include/linux/clk-provider.h, but lacks DT bindings for direct declaration. To use them, a lock is required to avoid operation conflict on a same register.

Add a clock controller to manage this lock.

---
 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
drivers/clk/Makefile +1 -0
index e3ca0d058a25..6cf0a888b673 100644
@@ -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
drivers/clk/clk-of.c new file +292 -0

292 lines not on this page · raw message

drivers/clk/clk-of.h new file +26 -0
new file mode 100644
index 000000000000..ddb1e57ec2f1
@@ -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);
Signature · 1 line
2.39.2
#5 lore raw

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

kernel test robot noticed the following build errors:

Hi David,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7a934f4bd7d6f9da84c8812da3ba42ee10f5778e]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Yang/dt-bindings-clock-Add-simple-clock-controller/20230417-034857
base:   7a934f4bd7d6f9da84c8812da3ba42ee10f5778e
patch link:    https://lore.kernel.org/r/20230416194624.1258860-3-mmyangfl%40gmail.com
patch subject: [PATCH v3 2/4] clk: Add simple clock controller
config: s390-randconfig-r036-20230416 (https://download.01.org/0day-ci/archive/20230419/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/242d6bab7339a967b40f9731b989913e8b2ea63c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Yang/dt-bindings-clock-Add-simple-clock-controller/20230417-034857 git checkout 242d6bab7339a967b40f9731b989913e8b2ea63c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

s390x-linux-ld: DWARF error: could not find abbrev number 115 kernel/dma/coherent.o: in function `dma_declare_coherent_memory':
coherent.c:(.text+0xc8): undefined reference to `memunmap'
s390x-linux-ld: kernel/dma/coherent.o: in function `dma_init_coherent_memory':
coherent.c:(.text+0x14a): undefined reference to `memremap'
s390x-linux-ld: coherent.c:(.text+0x254): undefined reference to `memunmap'
s390x-linux-ld: kernel/dma/coherent.o: in function `dma_release_coherent_memory':
coherent.c:(.text+0x2c0): undefined reference to `memunmap'
s390x-linux-ld: DWARF error: could not find abbrev number 53 drivers/clk/clk-of.o: in function `of_clk_get_reg':

>> clk-of.c:(.text+0x6a): undefined reference to `of_iomap'

s390x-linux-ld: drivers/clk/clk-of.o: in function `of_crg_ctrl_setup':
clk-of.c:(.text+0x322): undefined reference to `of_iomap'
s390x-linux-ld: DWARF error: could not find abbrev number 8446 drivers/clk/clk-fixed-mmio.o: in function `fixed_mmio_clk_setup':
clk-fixed-mmio.c:(.text+0x48): undefined reference to `of_iomap'
s390x-linux-ld: clk-fixed-mmio.c:(.text+0x68): undefined reference to `iounmap'
s390x-linux-ld: DWARF error: could not find abbrev number 15506 drivers/char/xillybus/xillybus_of.o: in function `xilly_drv_probe':
xillybus_of.c:(.text+0x64): undefined reference to `devm_platform_ioremap_resource'
s390x-linux-ld: DWARF error: could not find abbrev number 93 drivers/misc/open-dice.o: in function `open_dice_write':
open-dice.c:(.text+0xe6): undefined reference to `devm_memremap'
s390x-linux-ld: open-dice.c:(.text+0x176): undefined reference to `devm_memunmap'
s390x-linux-ld: DWARF error: could not find abbrev number 67 drivers/pcmcia/cistpl.o: in function `release_cis_mem':
cistpl.c:(.text+0xe8): undefined reference to `iounmap'
s390x-linux-ld: drivers/pcmcia/cistpl.o: in function `set_cis_map':
cistpl.c:(.text+0x4a6): undefined reference to `ioremap'
s390x-linux-ld: cistpl.c:(.text+0x520): undefined reference to `iounmap'
s390x-linux-ld: cistpl.c:(.text+0x572): undefined reference to `iounmap'
s390x-linux-ld: cistpl.c:(.text+0x59c): undefined reference to `ioremap'

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
author
#6 lore raw

PATCH v3 3/4 dt-bindings: clock: Add gate-clock

In reply to David Yang

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

---
 .../devicetree/bindings/clock/gate-clock.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.yaml
Documentation/devicetree/bindings/clock/gate-clock.yaml new file +59 -0
new file mode 100644
index 000000000000..bcd549dd9db1
@@ -0,0 +1,59 @@
+# 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 <[email protected]>
+
+description: |
+  Clock which can gate its output, by toggling one bit in a register. Such
+  register may also control other clocks or reset requests.
+
+  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: Documentation/devicetree/bindings/clock/simple-clock-controller.yaml
+
+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:
+  - |
+    clock {
+      compatible = "gate-clock";
+      #clock-cells = <0>;
+      offset = <0xcc>;
+      bits = <3>;
+      clock-output-names = "my-clk";
+    };
Signature · 1 line
2.39.2
maintainer NAK
#7 lore raw

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

Quoting David Yang (2023-04-16 12:46:21)

Quoted 4 lines

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

---

NAK

We've been over this many times. The binding will not be accepted.

author
#8 lore raw

PATCH v3 4/4 clk: gate: Add DT binding

In reply to David Yang

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

---
 drivers/clk/clk-gate.c | 81 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
drivers/clk/clk-gate.c +81 -0
index 64283807600b..a70df4a2a9a7 100644
@@ -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
Signature · 1 line
2.39.2
#9 lore raw
In reply to David Yang
Quoted 4 lines · David Yang

These clocks were provided in include/linux/clk-provider.h, but lacks DT bindings. Add DT bindings for them.

v2: split clock controller and its clocks

I gave you several comments on v1 and this vague changelog entry suggests you ignored all of it.

Implement feedback and respond to each one that you ack it or disagree with it.

Best regards,
Krzysztof

author
#10 lore raw
Quoted 10 lines · Krzysztof Kozlowski <[email protected]> 于2023年4月17日周一 14:27写道:
Quoted 4 lines · David Yang

These clocks were provided in include/linux/clk-provider.h, but lacks DT bindings. Add DT bindings for them.

v2: split clock controller and its clocks

I gave you several comments on v1 and this vague changelog entry suggests you ignored all of it.

Quoted 7 lines

Implement feedback and respond to each one that you ack it or disagree with it.

Best regards,
Krzysztof

All issues have been addressed in v3 patch. I omitted details in changelog to reduce redundancy. If that is not the proper way to make response in dt maillist, please let me know.

Responses for each comments in v1 and v2 have been sent against original email.

maintainer
#11 lore raw
Quoted 19 lines · Yangfl
Quoted 10 lines · Krzysztof Kozlowski <[email protected]> 于2023年4月17日周一 14:27写道:
Quoted 4 lines · David Yang

These clocks were provided in include/linux/clk-provider.h, but lacks DT bindings. Add DT bindings for them.

v2: split clock controller and its clocks

I gave you several comments on v1 and this vague changelog entry suggests you ignored all of it.

Quoted 7 lines

Implement feedback and respond to each one that you ack it or disagree with it.

Best regards,
Krzysztof

Quoted 5 lines

All issues have been addressed in v3 patch. I omitted details in changelog to reduce redundancy. If that is not the proper way to make response in dt maillist, please let me know.

Responses for each comments in v1 and v2 have been sent against original email.

Go look at v1 replies. This series is a hard no.

Rob