[PATCH 0/3] Apple M1 clock gate driver

[PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi,

This series adds support for the clock gates present in Apple's SoCs such as
the M1.

These SoCs have various clock gates for their peripherals usually located in
one or two MMIO regions. Each clock gate is a single 32 bit register which
contains bits for the requested and the actual mode. The mode is represented by
four bits. We however only care about "everything enabled" (0b1111) and
"everything disabled" (0b000) for now. The other modes are probably different
low-power states which don't even seem to be used by MacOS. The actual power
management is located in a different region (and probably handled by a separate
processor on the M1).

Additionally, these clocks have a topology that is usually specified in Apple's
Device Tree. As far as I can tell most Linux drivers seem to encode this topology
directly in the source code though. Despite this, we would still like to encode
the topology in the device tree itself:

Apple seems to only change HW blocks when they have a very good reason and even
then they seem to keep the changes minimal. This specific clock gate MMIO block
has already been used in the Apple A7 released in 2013. The only differences
since then are the available clocks (which can be identified by a simple offset)
and their topology.

It's likely that Apple will still use this block in future SoCs as well. By
encoding the clock gate topology inside the device tree as well we can use a
single driver and only need updates to the device tree once they are released.
This might allow end users to already run their favorite distribution by just
updating the bootloader with a new device tree instead of having to wait until
the new topology is integrated into their distro kernel.

Additionally, the driver itself can be kept very simple and not much code needs
to be duplicated and then updated for each new SoC between various consumers of
these device tree nodes (Linux, u-boot, and OpenBSD for now).


This series therefore creates a single device tree node for each clock gate.
This unfortunately maps a whole page out of which only a single register will
be used for each node.

The other alternative I considered was to create a syscon/simple-mfd node
and have the clock nodes as children. The gates would then use a regmap which
would only require a single entry in the pagetable for all clocks. I decided
against this option since the clock gate MMIO region actually isn't a
multi-function device.

I'll also gladly implement other solutions - especially if there is a way to
keep the clock toplogy inside the device tree without having to create a
pagetable entry for every single clock node.

Best,


Sven

Sven Peter (3):
  dt-bindings: clock: add DT bindings for apple,gate-clock
  clk: add support for gate clocks on Apple SoCs
  arm64: apple: add uart gate clocks

 .../bindings/clock/apple,gate-clock.yaml      |  60 +++++++
 MAINTAINERS                                   |   2 +
 arch/arm64/boot/dts/apple/t8103.dtsi          |  36 ++++-
 drivers/clk/Kconfig                           |  12 ++
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-apple-gate.c                  | 152 ++++++++++++++++++
 6 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 create mode 100644 drivers/clk/clk-apple-gate.c

-- 
2.25.1

[PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi,

This series adds support for the clock gates present in Apple's SoCs such as
the M1.

These SoCs have various clock gates for their peripherals usually located in
one or two MMIO regions. Each clock gate is a single 32 bit register which
contains bits for the requested and the actual mode. The mode is represented by
four bits. We however only care about "everything enabled" (0b1111) and
"everything disabled" (0b000) for now. The other modes are probably different
low-power states which don't even seem to be used by MacOS. The actual power
management is located in a different region (and probably handled by a separate
processor on the M1).

Additionally, these clocks have a topology that is usually specified in Apple's
Device Tree. As far as I can tell most Linux drivers seem to encode this topology
directly in the source code though. Despite this, we would still like to encode
the topology in the device tree itself:

Apple seems to only change HW blocks when they have a very good reason and even
then they seem to keep the changes minimal. This specific clock gate MMIO block
has already been used in the Apple A7 released in 2013. The only differences
since then are the available clocks (which can be identified by a simple offset)
and their topology.

It's likely that Apple will still use this block in future SoCs as well. By
encoding the clock gate topology inside the device tree as well we can use a
single driver and only need updates to the device tree once they are released.
This might allow end users to already run their favorite distribution by just
updating the bootloader with a new device tree instead of having to wait until
the new topology is integrated into their distro kernel.

Additionally, the driver itself can be kept very simple and not much code needs
to be duplicated and then updated for each new SoC between various consumers of
these device tree nodes (Linux, u-boot, and OpenBSD for now).


This series therefore creates a single device tree node for each clock gate.
This unfortunately maps a whole page out of which only a single register will
be used for each node.

The other alternative I considered was to create a syscon/simple-mfd node
and have the clock nodes as children. The gates would then use a regmap which
would only require a single entry in the pagetable for all clocks. I decided
against this option since the clock gate MMIO region actually isn't a
multi-function device.

I'll also gladly implement other solutions - especially if there is a way to
keep the clock toplogy inside the device tree without having to create a
pagetable entry for every single clock node.

Best,


Sven

Sven Peter (3):
  dt-bindings: clock: add DT bindings for apple,gate-clock
  clk: add support for gate clocks on Apple SoCs
  arm64: apple: add uart gate clocks

 .../bindings/clock/apple,gate-clock.yaml      |  60 +++++++
 MAINTAINERS                                   |   2 +
 arch/arm64/boot/dts/apple/t8103.dtsi          |  36 ++++-
 drivers/clk/Kconfig                           |  12 ++
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-apple-gate.c                  | 152 ++++++++++++++++++
 6 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 create mode 100644 drivers/clk/clk-apple-gate.c

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] dt-bindings: clock: add DT bindings for apple,gate-clock

[Sven Peter]

These gated clocks are found on Apple SoCs, such as the M1, and
are required to enable access to MMIO regions of various peripherals.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 .../bindings/clock/apple,gate-clock.yaml      | 60 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,gate-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml b/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
new file mode 100644
index 000000000000..3aae47c40b42
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/apple,gate-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for Apple clock gates
+
+maintainers:
+  - Sven Peter <sven@svenpeter.dev>
+
+description: |
+  Apple SoC's such as the M1 contain various clock gates.
+  These clock gates do not have a frequency associated with them and are only
+  used to power on/off various peripherals. Generally, a clock gate needs to
+  be enabled before the respective MMIO region can be accessed.
+
+  Each clock gate is configured by a single 32bit MMIO register which contains
+  the actual and the target state. The state is encoded as four bits but
+  right now only "powered on" / 0b1111 and "powered off" / 0b0000 are used.
+
+
+properties:
+  compatible:
+    enum:
+      - apple,t8103-gate-clock
+      - apple,gate-clock
+
+  "#clock-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    maxItems: 1
+
+  clock-output-names:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clock-output-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    clock@3b7001c0 {
+      compatible = "apple,t8103-gate-clock";
+      reg = <0x3b7001c0 0x4>;
+      #clock-cells = <0>;
+      clock-output-names = "sio_busif_clk";
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 008fcad7ac00..59c026ce4d73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1654,6 +1654,7 @@ B:	https://github.com/AsahiLinux/linux/issues
 C:	irc://chat.freenode.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
+F:	Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
-- 
2.25.1

[PATCH 1/3] dt-bindings: clock: add DT bindings for apple,gate-clock

[Sven Peter]

These gated clocks are found on Apple SoCs, such as the M1, and
are required to enable access to MMIO regions of various peripherals.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 .../bindings/clock/apple,gate-clock.yaml      | 60 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,gate-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml b/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
new file mode 100644
index 000000000000..3aae47c40b42
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/apple,gate-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for Apple clock gates
+
+maintainers:
+  - Sven Peter <sven@svenpeter.dev>
+
+description: |
+  Apple SoC's such as the M1 contain various clock gates.
+  These clock gates do not have a frequency associated with them and are only
+  used to power on/off various peripherals. Generally, a clock gate needs to
+  be enabled before the respective MMIO region can be accessed.
+
+  Each clock gate is configured by a single 32bit MMIO register which contains
+  the actual and the target state. The state is encoded as four bits but
+  right now only "powered on" / 0b1111 and "powered off" / 0b0000 are used.
+
+
+properties:
+  compatible:
+    enum:
+      - apple,t8103-gate-clock
+      - apple,gate-clock
+
+  "#clock-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    maxItems: 1
+
+  clock-output-names:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clock-output-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    clock@3b7001c0 {
+      compatible = "apple,t8103-gate-clock";
+      reg = <0x3b7001c0 0x4>;
+      #clock-cells = <0>;
+      clock-output-names = "sio_busif_clk";
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 008fcad7ac00..59c026ce4d73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1654,6 +1654,7 @@ B:	https://github.com/AsahiLinux/linux/issues
 C:	irc://chat.freenode.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
+F:	Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Sven Peter]

Add a simple driver for gate clocks found on Apple SoCs. These don't
have any frequency associated with them and are only used to enable
access to MMIO regions of various peripherals.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 MAINTAINERS                  |   1 +
 drivers/clk/Kconfig          |  12 +++
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-apple-gate.c | 152 +++++++++++++++++++++++++++++++++++
 4 files changed, 166 insertions(+)
 create mode 100644 drivers/clk/clk-apple-gate.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 59c026ce4d73..4b5d8e7a0fbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1657,6 +1657,7 @@ F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/boot/dts/apple/
+F:	drivers/clk/clk-apple-gate.c
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e80918be8e9c..ac987a8cf318 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -245,6 +245,18 @@ config CLK_TWL6040
 	  McPDM. McPDM module is using the external bit clock on the McPDM bus
 	  as functional clock.
 
+config COMMON_CLK_APPLE
+	tristate "Clock driver for Apple platforms"
+	depends on ARCH_APPLE && COMMON_CLK
+	default ARCH_APPLE
+	help
+	  Support for clock gates on Apple SoCs such as the M1.
+
+	  These clock gates do not have a frequency associated with them and
+	  are only used to power on/off various peripherals. Generally, a clock
+	  gate needs to be enabled before the respective MMIO region can be
+	  accessed.
+
 config COMMON_CLK_AXI_CLKGEN
 	tristate "AXI clkgen driver"
 	depends on HAS_IOMEM || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 5f06879d7fe9..ba73960694e3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,7 @@ endif
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file path name
+obj-$(CONFIG_COMMON_CLK_APPLE)		+= clk-apple-gate.o
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
new file mode 100644
index 000000000000..799e9269758f
--- /dev/null
+++ b/drivers/clk/clk-apple-gate.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC clock/power gating driver
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+
+#define CLOCK_TARGET_MODE_MASK 0x0f
+#define CLOCK_TARGET_MODE(m) (((m)&0xf))
+#define CLOCK_ACTUAL_MODE_MASK 0xf0
+#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
+
+#define CLOCK_MODE_ENABLE 0xf
+#define CLOCK_MODE_DISABLE 0
+
+#define CLOCK_ENDISABLE_TIMEOUT 100
+
+struct apple_clk_gate {
+	struct clk_hw hw;
+	void __iomem *reg;
+};
+
+#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
+
+static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
+{
+	struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+	u32 reg;
+	u32 mode;
+
+	if (enable)
+		mode = CLOCK_MODE_ENABLE;
+	else
+		mode = CLOCK_MODE_DISABLE;
+
+	reg = readl(gate->reg);
+	reg &= ~CLOCK_TARGET_MODE_MASK;
+	reg |= CLOCK_TARGET_MODE(mode);
+	writel(reg, gate->reg);
+
+	return readl_poll_timeout_atomic(gate->reg, reg,
+					 (reg & CLOCK_ACTUAL_MODE_MASK) ==
+						 CLOCK_ACTUAL_MODE(mode),
+					 1, CLOCK_ENDISABLE_TIMEOUT);
+}
+
+static int apple_clk_gate_enable(struct clk_hw *hw)
+{
+	return apple_clk_gate_endisable(hw, 1);
+}
+
+static void apple_clk_gate_disable(struct clk_hw *hw)
+{
+	apple_clk_gate_endisable(hw, 0);
+}
+
+static int apple_clk_gate_is_enabled(struct clk_hw *hw)
+{
+	struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+	u32 reg;
+
+	reg = readl(gate->reg);
+
+	if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))
+		return 1;
+	return 0;
+}
+
+static const struct clk_ops apple_clk_gate_ops = {
+	.enable = apple_clk_gate_enable,
+	.disable = apple_clk_gate_disable,
+	.is_enabled = apple_clk_gate_is_enabled,
+};
+
+static int apple_gate_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const struct clk_parent_data parent_data[] = {
+		{ .index = 0 },
+	};
+	struct apple_clk_gate *data;
+	struct clk_hw *hw;
+	struct clk_init_data init;
+	struct resource *res;
+	int num_parents;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->reg))
+		return PTR_ERR(data->reg);
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents > 1) {
+		dev_err(dev, "clock supports at most one parent\n");
+		return -EINVAL;
+	}
+
+	init.name = dev->of_node->name;
+	init.ops = &apple_clk_gate_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.parent_data = parent_data;
+	init.num_parents = num_parents;
+
+	data->hw.init = &init;
+	hw = &data->hw;
+
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+}
+
+static const struct of_device_id apple_gate_clk_of_match[] = {
+	{ .compatible = "apple,t8103-gate-clock" },
+	{ .compatible = "apple,gate-clock" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, apple_gate_clk_of_match);
+
+static struct platform_driver apple_gate_clkdriver = {
+	.probe = apple_gate_clk_probe,
+	.driver = {
+		.name = "apple-gate-clock",
+		.of_match_table = apple_gate_clk_of_match,
+	},
+};
+
+MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
+MODULE_DESCRIPTION("Clock gating driver for Apple SoCs");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(apple_gate_clkdriver);
-- 
2.25.1

[PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Sven Peter]

Add a simple driver for gate clocks found on Apple SoCs. These don't
have any frequency associated with them and are only used to enable
access to MMIO regions of various peripherals.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 MAINTAINERS                  |   1 +
 drivers/clk/Kconfig          |  12 +++
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-apple-gate.c | 152 +++++++++++++++++++++++++++++++++++
 4 files changed, 166 insertions(+)
 create mode 100644 drivers/clk/clk-apple-gate.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 59c026ce4d73..4b5d8e7a0fbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1657,6 +1657,7 @@ F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/boot/dts/apple/
+F:	drivers/clk/clk-apple-gate.c
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e80918be8e9c..ac987a8cf318 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -245,6 +245,18 @@ config CLK_TWL6040
 	  McPDM. McPDM module is using the external bit clock on the McPDM bus
 	  as functional clock.
 
+config COMMON_CLK_APPLE
+	tristate "Clock driver for Apple platforms"
+	depends on ARCH_APPLE && COMMON_CLK
+	default ARCH_APPLE
+	help
+	  Support for clock gates on Apple SoCs such as the M1.
+
+	  These clock gates do not have a frequency associated with them and
+	  are only used to power on/off various peripherals. Generally, a clock
+	  gate needs to be enabled before the respective MMIO region can be
+	  accessed.
+
 config COMMON_CLK_AXI_CLKGEN
 	tristate "AXI clkgen driver"
 	depends on HAS_IOMEM || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 5f06879d7fe9..ba73960694e3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,7 @@ endif
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file path name
+obj-$(CONFIG_COMMON_CLK_APPLE)		+= clk-apple-gate.o
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
new file mode 100644
index 000000000000..799e9269758f
--- /dev/null
+++ b/drivers/clk/clk-apple-gate.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC clock/power gating driver
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+
+#define CLOCK_TARGET_MODE_MASK 0x0f
+#define CLOCK_TARGET_MODE(m) (((m)&0xf))
+#define CLOCK_ACTUAL_MODE_MASK 0xf0
+#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
+
+#define CLOCK_MODE_ENABLE 0xf
+#define CLOCK_MODE_DISABLE 0
+
+#define CLOCK_ENDISABLE_TIMEOUT 100
+
+struct apple_clk_gate {
+	struct clk_hw hw;
+	void __iomem *reg;
+};
+
+#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
+
+static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
+{
+	struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+	u32 reg;
+	u32 mode;
+
+	if (enable)
+		mode = CLOCK_MODE_ENABLE;
+	else
+		mode = CLOCK_MODE_DISABLE;
+
+	reg = readl(gate->reg);
+	reg &= ~CLOCK_TARGET_MODE_MASK;
+	reg |= CLOCK_TARGET_MODE(mode);
+	writel(reg, gate->reg);
+
+	return readl_poll_timeout_atomic(gate->reg, reg,
+					 (reg & CLOCK_ACTUAL_MODE_MASK) ==
+						 CLOCK_ACTUAL_MODE(mode),
+					 1, CLOCK_ENDISABLE_TIMEOUT);
+}
+
+static int apple_clk_gate_enable(struct clk_hw *hw)
+{
+	return apple_clk_gate_endisable(hw, 1);
+}
+
+static void apple_clk_gate_disable(struct clk_hw *hw)
+{
+	apple_clk_gate_endisable(hw, 0);
+}
+
+static int apple_clk_gate_is_enabled(struct clk_hw *hw)
+{
+	struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+	u32 reg;
+
+	reg = readl(gate->reg);
+
+	if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))
+		return 1;
+	return 0;
+}
+
+static const struct clk_ops apple_clk_gate_ops = {
+	.enable = apple_clk_gate_enable,
+	.disable = apple_clk_gate_disable,
+	.is_enabled = apple_clk_gate_is_enabled,
+};
+
+static int apple_gate_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const struct clk_parent_data parent_data[] = {
+		{ .index = 0 },
+	};
+	struct apple_clk_gate *data;
+	struct clk_hw *hw;
+	struct clk_init_data init;
+	struct resource *res;
+	int num_parents;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->reg))
+		return PTR_ERR(data->reg);
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents > 1) {
+		dev_err(dev, "clock supports at most one parent\n");
+		return -EINVAL;
+	}
+
+	init.name = dev->of_node->name;
+	init.ops = &apple_clk_gate_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.parent_data = parent_data;
+	init.num_parents = num_parents;
+
+	data->hw.init = &init;
+	hw = &data->hw;
+
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+}
+
+static const struct of_device_id apple_gate_clk_of_match[] = {
+	{ .compatible = "apple,t8103-gate-clock" },
+	{ .compatible = "apple,gate-clock" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, apple_gate_clk_of_match);
+
+static struct platform_driver apple_gate_clkdriver = {
+	.probe = apple_gate_clk_probe,
+	.driver = {
+		.name = "apple-gate-clock",
+		.of_match_table = apple_gate_clk_of_match,
+	},
+};
+
+MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
+MODULE_DESCRIPTION("Clock gating driver for Apple SoCs");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(apple_gate_clkdriver);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Stephen Boyd]

Quoting Sven Peter (2021-05-24 11:27:44)
> Add a simple driver for gate clocks found on Apple SoCs. These don't
> have any frequency associated with them and are only used to enable
> access to MMIO regions of various peripherals.

Can we figure out what frequency they are clocking at?

> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index e80918be8e9c..ac987a8cf318 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -245,6 +245,18 @@ config CLK_TWL6040
>           McPDM. McPDM module is using the external bit clock on the McPDM bus
>           as functional clock.
>  
> +config COMMON_CLK_APPLE
> +       tristate "Clock driver for Apple platforms"
> +       depends on ARCH_APPLE && COMMON_CLK

The COMMON_CLK depend is redundant. Please remove.

> +       default ARCH_APPLE
> +       help
> +         Support for clock gates on Apple SoCs such as the M1.
> +
> +         These clock gates do not have a frequency associated with them and
> +         are only used to power on/off various peripherals. Generally, a clock
> +         gate needs to be enabled before the respective MMIO region can be
> +         accessed.
> +
>  config COMMON_CLK_AXI_CLKGEN
>         tristate "AXI clkgen driver"
>         depends on HAS_IOMEM || COMPILE_TEST
> diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
> new file mode 100644
> index 000000000000..799e9269758f
> --- /dev/null
> +++ b/drivers/clk/clk-apple-gate.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Apple SoC clock/power gating driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/clk.h>

Hopefully this can be droped.

> +#include <linux/clkdev.h>

And this one too. clkdev is not modern.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +
> +#define CLOCK_TARGET_MODE_MASK 0x0f

APPLE_ prefix on all these?

> +#define CLOCK_TARGET_MODE(m) (((m)&0xf))
> +#define CLOCK_ACTUAL_MODE_MASK 0xf0
> +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
> +
> +#define CLOCK_MODE_ENABLE 0xf
> +#define CLOCK_MODE_DISABLE 0
> +
> +#define CLOCK_ENDISABLE_TIMEOUT 100
> +
> +struct apple_clk_gate {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +};
> +
> +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
> +
> +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> +       u32 reg;
> +       u32 mode;
> +
> +       if (enable)
> +               mode = CLOCK_MODE_ENABLE;
> +       else
> +               mode = CLOCK_MODE_DISABLE;
> +
> +       reg = readl(gate->reg);
> +       reg &= ~CLOCK_TARGET_MODE_MASK;
> +       reg |= CLOCK_TARGET_MODE(mode);
> +       writel(reg, gate->reg);
> +
> +       return readl_poll_timeout_atomic(gate->reg, reg,
> +                                        (reg & CLOCK_ACTUAL_MODE_MASK) ==
> +                                                CLOCK_ACTUAL_MODE(mode),
> +                                        1, CLOCK_ENDISABLE_TIMEOUT);

Maybe this

          return readl_poll_timeout_atomic(gate->reg, reg,
		   (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode),
		   1, CLOCK_ENDISABLE_TIMEOUT);

at the least please don't break the == across two lines.

> +}
> +
> +static int apple_clk_gate_enable(struct clk_hw *hw)
> +{
> +       return apple_clk_gate_endisable(hw, 1);
> +}
> +
> +static void apple_clk_gate_disable(struct clk_hw *hw)
> +{
> +       apple_clk_gate_endisable(hw, 0);
> +}
> +
> +static int apple_clk_gate_is_enabled(struct clk_hw *hw)
> +{
> +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> +       u32 reg;
> +
> +       reg = readl(gate->reg);
> +
> +       if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))

Any chance we can use FIELD_GET() and friends? Please don't do bit
shifting stuff inside conditionals as it just makes life more
complicated than it needs to be.

> +               return 1;
> +       return 0;

How about just return <logic>?

> +}
> +
> +static const struct clk_ops apple_clk_gate_ops = {
> +       .enable = apple_clk_gate_enable,
> +       .disable = apple_clk_gate_disable,
> +       .is_enabled = apple_clk_gate_is_enabled,
> +};
> +
> +static int apple_gate_clk_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->of_node;
> +       const struct clk_parent_data parent_data[] = {
> +               { .index = 0 },
> +       };

Yay thanks for not doing it the old way!

> +       struct apple_clk_gate *data;
> +       struct clk_hw *hw;
> +       struct clk_init_data init;
> +       struct resource *res;
> +       int num_parents;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       data->reg = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(data->reg))
> +               return PTR_ERR(data->reg);
> +
> +       num_parents = of_clk_get_parent_count(node);

Oh no I spoke too soon.

> +       if (num_parents > 1) {
> +               dev_err(dev, "clock supports at most one parent\n");
> +               return -EINVAL;
> +       }
> +
> +       init.name = dev->of_node->name;
> +       init.ops = &apple_clk_gate_ops;
> +       init.flags = 0;
> +       init.parent_names = NULL;
> +       init.parent_data = parent_data;
> +       init.num_parents = num_parents;
> +
> +       data->hw.init = &init;
> +       hw = &data->hw;
> +
> +       ret = devm_clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);

It looks like a one clk per DT node binding which is not how we do it. I
see Rob has started that discussion on the binding so we can keep that
there.

> +}
> +

Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Stephen Boyd]

Quoting Sven Peter (2021-05-24 11:27:44)
> Add a simple driver for gate clocks found on Apple SoCs. These don't
> have any frequency associated with them and are only used to enable
> access to MMIO regions of various peripherals.

Can we figure out what frequency they are clocking at?

> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index e80918be8e9c..ac987a8cf318 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -245,6 +245,18 @@ config CLK_TWL6040
>           McPDM. McPDM module is using the external bit clock on the McPDM bus
>           as functional clock.
>  
> +config COMMON_CLK_APPLE
> +       tristate "Clock driver for Apple platforms"
> +       depends on ARCH_APPLE && COMMON_CLK

The COMMON_CLK depend is redundant. Please remove.

> +       default ARCH_APPLE
> +       help
> +         Support for clock gates on Apple SoCs such as the M1.
> +
> +         These clock gates do not have a frequency associated with them and
> +         are only used to power on/off various peripherals. Generally, a clock
> +         gate needs to be enabled before the respective MMIO region can be
> +         accessed.
> +
>  config COMMON_CLK_AXI_CLKGEN
>         tristate "AXI clkgen driver"
>         depends on HAS_IOMEM || COMPILE_TEST
> diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
> new file mode 100644
> index 000000000000..799e9269758f
> --- /dev/null
> +++ b/drivers/clk/clk-apple-gate.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Apple SoC clock/power gating driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/clk.h>

Hopefully this can be droped.

> +#include <linux/clkdev.h>

And this one too. clkdev is not modern.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +
> +#define CLOCK_TARGET_MODE_MASK 0x0f

APPLE_ prefix on all these?

> +#define CLOCK_TARGET_MODE(m) (((m)&0xf))
> +#define CLOCK_ACTUAL_MODE_MASK 0xf0
> +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
> +
> +#define CLOCK_MODE_ENABLE 0xf
> +#define CLOCK_MODE_DISABLE 0
> +
> +#define CLOCK_ENDISABLE_TIMEOUT 100
> +
> +struct apple_clk_gate {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +};
> +
> +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
> +
> +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> +       u32 reg;
> +       u32 mode;
> +
> +       if (enable)
> +               mode = CLOCK_MODE_ENABLE;
> +       else
> +               mode = CLOCK_MODE_DISABLE;
> +
> +       reg = readl(gate->reg);
> +       reg &= ~CLOCK_TARGET_MODE_MASK;
> +       reg |= CLOCK_TARGET_MODE(mode);
> +       writel(reg, gate->reg);
> +
> +       return readl_poll_timeout_atomic(gate->reg, reg,
> +                                        (reg & CLOCK_ACTUAL_MODE_MASK) ==
> +                                                CLOCK_ACTUAL_MODE(mode),
> +                                        1, CLOCK_ENDISABLE_TIMEOUT);

Maybe this

          return readl_poll_timeout_atomic(gate->reg, reg,
		   (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode),
		   1, CLOCK_ENDISABLE_TIMEOUT);

at the least please don't break the == across two lines.

> +}
> +
> +static int apple_clk_gate_enable(struct clk_hw *hw)
> +{
> +       return apple_clk_gate_endisable(hw, 1);
> +}
> +
> +static void apple_clk_gate_disable(struct clk_hw *hw)
> +{
> +       apple_clk_gate_endisable(hw, 0);
> +}
> +
> +static int apple_clk_gate_is_enabled(struct clk_hw *hw)
> +{
> +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> +       u32 reg;
> +
> +       reg = readl(gate->reg);
> +
> +       if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))

Any chance we can use FIELD_GET() and friends? Please don't do bit
shifting stuff inside conditionals as it just makes life more
complicated than it needs to be.

> +               return 1;
> +       return 0;

How about just return <logic>?

> +}
> +
> +static const struct clk_ops apple_clk_gate_ops = {
> +       .enable = apple_clk_gate_enable,
> +       .disable = apple_clk_gate_disable,
> +       .is_enabled = apple_clk_gate_is_enabled,
> +};
> +
> +static int apple_gate_clk_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->of_node;
> +       const struct clk_parent_data parent_data[] = {
> +               { .index = 0 },
> +       };

Yay thanks for not doing it the old way!

> +       struct apple_clk_gate *data;
> +       struct clk_hw *hw;
> +       struct clk_init_data init;
> +       struct resource *res;
> +       int num_parents;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       data->reg = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(data->reg))
> +               return PTR_ERR(data->reg);
> +
> +       num_parents = of_clk_get_parent_count(node);

Oh no I spoke too soon.

> +       if (num_parents > 1) {
> +               dev_err(dev, "clock supports at most one parent\n");
> +               return -EINVAL;
> +       }
> +
> +       init.name = dev->of_node->name;
> +       init.ops = &apple_clk_gate_ops;
> +       init.flags = 0;
> +       init.parent_names = NULL;
> +       init.parent_data = parent_data;
> +       init.num_parents = num_parents;
> +
> +       data->hw.init = &init;
> +       hw = &data->hw;
> +
> +       ret = devm_clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);

It looks like a one clk per DT node binding which is not how we do it. I
see Rob has started that discussion on the binding so we can keep that
there.

> +}
> +

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Sven Peter]

Hi,

Thanks for the review!

On Wed, May 26, 2021, at 05:09, Stephen Boyd wrote:
> Quoting Sven Peter (2021-05-24 11:27:44)
> > Add a simple driver for gate clocks found on Apple SoCs. These don't
> > have any frequency associated with them and are only used to enable
> > access to MMIO regions of various peripherals.
> 
> Can we figure out what frequency they are clocking at?
> 

I don't think so. I've written some more details about how Apple
uses the clocks in a reply to Rob, but the short version is that
these clock gates are only used as on/off switches. There are
separate clocks that actually have a frequency associated with
them.


> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index e80918be8e9c..ac987a8cf318 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -245,6 +245,18 @@ config CLK_TWL6040
> >           McPDM. McPDM module is using the external bit clock on the McPDM bus
> >           as functional clock.
> >  
> > +config COMMON_CLK_APPLE
> > +       tristate "Clock driver for Apple platforms"
> > +       depends on ARCH_APPLE && COMMON_CLK
> 
> The COMMON_CLK depend is redundant. Please remove.

Removed for v2.

> 
> > +       default ARCH_APPLE
> > +       help
> > +         Support for clock gates on Apple SoCs such as the M1.
> > +
> > +         These clock gates do not have a frequency associated with them and
> > +         are only used to power on/off various peripherals. Generally, a clock
> > +         gate needs to be enabled before the respective MMIO region can be
> > +         accessed.
> > +
> >  config COMMON_CLK_AXI_CLKGEN
> >         tristate "AXI clkgen driver"
> >         depends on HAS_IOMEM || COMPILE_TEST
> > diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
> > new file mode 100644
> > index 000000000000..799e9269758f
> > --- /dev/null
> > +++ b/drivers/clk/clk-apple-gate.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Apple SoC clock/power gating driver
> > + *
> > + * Copyright The Asahi Linux Contributors
> > + */
> > +
> > +#include <linux/clk.h>
> 
> Hopefully this can be droped.
> 
> > +#include <linux/clkdev.h>
> 
> And this one too. clkdev is not modern.

Okay, will remove both headers (and also fix any code that uses them).

> 
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +
> > +#define CLOCK_TARGET_MODE_MASK 0x0f
> 
> APPLE_ prefix on all these?

Fixed for v2.

> 
> > +#define CLOCK_TARGET_MODE(m) (((m)&0xf))
> > +#define CLOCK_ACTUAL_MODE_MASK 0xf0
> > +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
> > +
> > +#define CLOCK_MODE_ENABLE 0xf
> > +#define CLOCK_MODE_DISABLE 0
> > +
> > +#define CLOCK_ENDISABLE_TIMEOUT 100
> > +
> > +struct apple_clk_gate {
> > +       struct clk_hw hw;
> > +       void __iomem *reg;
> > +};
> > +
> > +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
> > +
> > +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
> > +{
> > +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> > +       u32 reg;
> > +       u32 mode;
> > +
> > +       if (enable)
> > +               mode = CLOCK_MODE_ENABLE;
> > +       else
> > +               mode = CLOCK_MODE_DISABLE;
> > +
> > +       reg = readl(gate->reg);
> > +       reg &= ~CLOCK_TARGET_MODE_MASK;
> > +       reg |= CLOCK_TARGET_MODE(mode);
> > +       writel(reg, gate->reg);
> > +
> > +       return readl_poll_timeout_atomic(gate->reg, reg,
> > +                                        (reg & CLOCK_ACTUAL_MODE_MASK) ==
> > +                                                CLOCK_ACTUAL_MODE(mode),
> > +                                        1, CLOCK_ENDISABLE_TIMEOUT);
> 
> Maybe this
> 
>           return readl_poll_timeout_atomic(gate->reg, reg,
> 		   (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode),
> 		   1, CLOCK_ENDISABLE_TIMEOUT);
> 
> at the least please don't break the == across two lines.

Ah, sorry. I ran clang-format at the end and didn't catch that line when
I did my final review.
I'll use your suggestion for v2.

> 
> > +}
> > +
> > +static int apple_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +       return apple_clk_gate_endisable(hw, 1);
> > +}
> > +
> > +static void apple_clk_gate_disable(struct clk_hw *hw)
> > +{
> > +       apple_clk_gate_endisable(hw, 0);
> > +}
> > +
> > +static int apple_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> > +       u32 reg;
> > +
> > +       reg = readl(gate->reg);
> > +
> > +       if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))
> 
> Any chance we can use FIELD_GET() and friends? Please don't do bit
> shifting stuff inside conditionals as it just makes life more
> complicated than it needs to be.

Absolutely, thanks for the hint. I didn't know about FIELD_GET and will
use it for v2.

> 
> > +               return 1;
> > +       return 0;
> 
> How about just return <logic>?

Good point, fixed for v2.

> 
> > +}
> > +
> > +static const struct clk_ops apple_clk_gate_ops = {
> > +       .enable = apple_clk_gate_enable,
> > +       .disable = apple_clk_gate_disable,
> > +       .is_enabled = apple_clk_gate_is_enabled,
> > +};
> > +
> > +static int apple_gate_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *node = dev->of_node;
> > +       const struct clk_parent_data parent_data[] = {
> > +               { .index = 0 },
> > +       };
> 
> Yay thanks for not doing it the old way!

:)

> 
> > +       struct apple_clk_gate *data;
> > +       struct clk_hw *hw;
> > +       struct clk_init_data init;
> > +       struct resource *res;
> > +       int num_parents;
> > +       int ret;
> > +
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       data->reg = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(data->reg))
> > +               return PTR_ERR(data->reg);
> > +
> > +       num_parents = of_clk_get_parent_count(node);
> 
> Oh no I spoke too soon.

:(

Sorry, will fix it for v2 to use the new way.

> 
> > +       if (num_parents > 1) {
> > +               dev_err(dev, "clock supports at most one parent\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       init.name = dev->of_node->name;
> > +       init.ops = &apple_clk_gate_ops;
> > +       init.flags = 0;
> > +       init.parent_names = NULL;
> > +       init.parent_data = parent_data;
> > +       init.num_parents = num_parents;
> > +
> > +       data->hw.init = &init;
> > +       hw = &data->hw;
> > +
> > +       ret = devm_clk_hw_register(dev, hw);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
> 
> It looks like a one clk per DT node binding which is not how we do it. I
> see Rob has started that discussion on the binding so we can keep that
> there.
> 
> > +}
> > +
> 

Thanks,


Sven

Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs

[Sven Peter]

Hi,

Thanks for the review!

On Wed, May 26, 2021, at 05:09, Stephen Boyd wrote:
> Quoting Sven Peter (2021-05-24 11:27:44)
> > Add a simple driver for gate clocks found on Apple SoCs. These don't
> > have any frequency associated with them and are only used to enable
> > access to MMIO regions of various peripherals.
> 
> Can we figure out what frequency they are clocking at?
> 

I don't think so. I've written some more details about how Apple
uses the clocks in a reply to Rob, but the short version is that
these clock gates are only used as on/off switches. There are
separate clocks that actually have a frequency associated with
them.


> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index e80918be8e9c..ac987a8cf318 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -245,6 +245,18 @@ config CLK_TWL6040
> >           McPDM. McPDM module is using the external bit clock on the McPDM bus
> >           as functional clock.
> >  
> > +config COMMON_CLK_APPLE
> > +       tristate "Clock driver for Apple platforms"
> > +       depends on ARCH_APPLE && COMMON_CLK
> 
> The COMMON_CLK depend is redundant. Please remove.

Removed for v2.

> 
> > +       default ARCH_APPLE
> > +       help
> > +         Support for clock gates on Apple SoCs such as the M1.
> > +
> > +         These clock gates do not have a frequency associated with them and
> > +         are only used to power on/off various peripherals. Generally, a clock
> > +         gate needs to be enabled before the respective MMIO region can be
> > +         accessed.
> > +
> >  config COMMON_CLK_AXI_CLKGEN
> >         tristate "AXI clkgen driver"
> >         depends on HAS_IOMEM || COMPILE_TEST
> > diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
> > new file mode 100644
> > index 000000000000..799e9269758f
> > --- /dev/null
> > +++ b/drivers/clk/clk-apple-gate.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Apple SoC clock/power gating driver
> > + *
> > + * Copyright The Asahi Linux Contributors
> > + */
> > +
> > +#include <linux/clk.h>
> 
> Hopefully this can be droped.
> 
> > +#include <linux/clkdev.h>
> 
> And this one too. clkdev is not modern.

Okay, will remove both headers (and also fix any code that uses them).

> 
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +
> > +#define CLOCK_TARGET_MODE_MASK 0x0f
> 
> APPLE_ prefix on all these?

Fixed for v2.

> 
> > +#define CLOCK_TARGET_MODE(m) (((m)&0xf))
> > +#define CLOCK_ACTUAL_MODE_MASK 0xf0
> > +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
> > +
> > +#define CLOCK_MODE_ENABLE 0xf
> > +#define CLOCK_MODE_DISABLE 0
> > +
> > +#define CLOCK_ENDISABLE_TIMEOUT 100
> > +
> > +struct apple_clk_gate {
> > +       struct clk_hw hw;
> > +       void __iomem *reg;
> > +};
> > +
> > +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
> > +
> > +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
> > +{
> > +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> > +       u32 reg;
> > +       u32 mode;
> > +
> > +       if (enable)
> > +               mode = CLOCK_MODE_ENABLE;
> > +       else
> > +               mode = CLOCK_MODE_DISABLE;
> > +
> > +       reg = readl(gate->reg);
> > +       reg &= ~CLOCK_TARGET_MODE_MASK;
> > +       reg |= CLOCK_TARGET_MODE(mode);
> > +       writel(reg, gate->reg);
> > +
> > +       return readl_poll_timeout_atomic(gate->reg, reg,
> > +                                        (reg & CLOCK_ACTUAL_MODE_MASK) ==
> > +                                                CLOCK_ACTUAL_MODE(mode),
> > +                                        1, CLOCK_ENDISABLE_TIMEOUT);
> 
> Maybe this
> 
>           return readl_poll_timeout_atomic(gate->reg, reg,
> 		   (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode),
> 		   1, CLOCK_ENDISABLE_TIMEOUT);
> 
> at the least please don't break the == across two lines.

Ah, sorry. I ran clang-format at the end and didn't catch that line when
I did my final review.
I'll use your suggestion for v2.

> 
> > +}
> > +
> > +static int apple_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +       return apple_clk_gate_endisable(hw, 1);
> > +}
> > +
> > +static void apple_clk_gate_disable(struct clk_hw *hw)
> > +{
> > +       apple_clk_gate_endisable(hw, 0);
> > +}
> > +
> > +static int apple_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
> > +       u32 reg;
> > +
> > +       reg = readl(gate->reg);
> > +
> > +       if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))
> 
> Any chance we can use FIELD_GET() and friends? Please don't do bit
> shifting stuff inside conditionals as it just makes life more
> complicated than it needs to be.

Absolutely, thanks for the hint. I didn't know about FIELD_GET and will
use it for v2.

> 
> > +               return 1;
> > +       return 0;
> 
> How about just return <logic>?

Good point, fixed for v2.

> 
> > +}
> > +
> > +static const struct clk_ops apple_clk_gate_ops = {
> > +       .enable = apple_clk_gate_enable,
> > +       .disable = apple_clk_gate_disable,
> > +       .is_enabled = apple_clk_gate_is_enabled,
> > +};
> > +
> > +static int apple_gate_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *node = dev->of_node;
> > +       const struct clk_parent_data parent_data[] = {
> > +               { .index = 0 },
> > +       };
> 
> Yay thanks for not doing it the old way!

:)

> 
> > +       struct apple_clk_gate *data;
> > +       struct clk_hw *hw;
> > +       struct clk_init_data init;
> > +       struct resource *res;
> > +       int num_parents;
> > +       int ret;
> > +
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       data->reg = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(data->reg))
> > +               return PTR_ERR(data->reg);
> > +
> > +       num_parents = of_clk_get_parent_count(node);
> 
> Oh no I spoke too soon.

:(

Sorry, will fix it for v2 to use the new way.

> 
> > +       if (num_parents > 1) {
> > +               dev_err(dev, "clock supports at most one parent\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       init.name = dev->of_node->name;
> > +       init.ops = &apple_clk_gate_ops;
> > +       init.flags = 0;
> > +       init.parent_names = NULL;
> > +       init.parent_data = parent_data;
> > +       init.num_parents = num_parents;
> > +
> > +       data->hw.init = &init;
> > +       hw = &data->hw;
> > +
> > +       ret = devm_clk_hw_register(dev, hw);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
> 
> It looks like a one clk per DT node binding which is not how we do it. I
> see Rob has started that discussion on the binding so we can keep that
> there.
> 
> > +}
> > +
> 

Thanks,


Sven

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: apple: add uart gate clocks

[Sven Peter]

Now that we have a driver for gate clocks add the proper topology for the
UART. These are already enabled by the bootloader but are part of the
clock topology used by devices yet to be implemented.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index a1e22a2ea2e5..b7c85b800efd 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -120,7 +120,7 @@ serial0: serial@235200000 {
 			 * TODO: figure out the clocking properly, there may
 			 * be a third selectable clock.
 			 */
-			clocks = <&clk24>, <&clk24>;
+			clocks = <&clock_uart0>, <&clk24>;
 			clock-names = "uart", "clk_uart_baud0";
 			status = "disabled";
 		};
@@ -131,5 +131,39 @@ aic: interrupt-controller@23b100000 {
 			interrupt-controller;
 			reg = <0x2 0x3b100000 0x0 0x8000>;
 		};
+
+		clock_sio_busif: clock-sio-busif@23b7001c0 {
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			reg = <0x2 0x3b7001c0 0x0 0x4>;
+			clock-output-names = "clock_sio_busif";
+		};
+
+		clock_sio: clock-sio@23b7001c8 {
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			reg = <0x2 0x3b7001c8 0x0 0x4>;
+			clocks = <&clock_sio_busif>;
+			clock-names = "clock_sio_busif";
+			clock-output-names = "clock_sio";
+		};
+
+		clock_uart_p: clock-uart-p@23b700220 {
+			reg = <0x2 0x3b700220 0 4>;
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			clock-output-names = "clock_uart_p";
+			clocks = <&clock_sio>;
+			clock-names = "clock_sio";
+		};
+
+		clock_uart0: clock-uart0@23b700270 {
+			reg = <0x2 0x3b700270 0 4>;
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			clock-output-names = "clock_uart0";
+			clocks = <&clock_uart_p>;
+			clock-names = "clock_uart_p";
+		};
 	};
 };
-- 
2.25.1

[PATCH 3/3] arm64: apple: add uart gate clocks

[Sven Peter]

Now that we have a driver for gate clocks add the proper topology for the
UART. These are already enabled by the bootloader but are part of the
clock topology used by devices yet to be implemented.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index a1e22a2ea2e5..b7c85b800efd 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -120,7 +120,7 @@ serial0: serial@235200000 {
 			 * TODO: figure out the clocking properly, there may
 			 * be a third selectable clock.
 			 */
-			clocks = <&clk24>, <&clk24>;
+			clocks = <&clock_uart0>, <&clk24>;
 			clock-names = "uart", "clk_uart_baud0";
 			status = "disabled";
 		};
@@ -131,5 +131,39 @@ aic: interrupt-controller@23b100000 {
 			interrupt-controller;
 			reg = <0x2 0x3b100000 0x0 0x8000>;
 		};
+
+		clock_sio_busif: clock-sio-busif@23b7001c0 {
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			reg = <0x2 0x3b7001c0 0x0 0x4>;
+			clock-output-names = "clock_sio_busif";
+		};
+
+		clock_sio: clock-sio@23b7001c8 {
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			reg = <0x2 0x3b7001c8 0x0 0x4>;
+			clocks = <&clock_sio_busif>;
+			clock-names = "clock_sio_busif";
+			clock-output-names = "clock_sio";
+		};
+
+		clock_uart_p: clock-uart-p@23b700220 {
+			reg = <0x2 0x3b700220 0 4>;
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			clock-output-names = "clock_uart_p";
+			clocks = <&clock_sio>;
+			clock-names = "clock_sio";
+		};
+
+		clock_uart0: clock-uart0@23b700270 {
+			reg = <0x2 0x3b700270 0 4>;
+			compatible = "apple,t8103-gate-clock";
+			#clock-cells = <0>;
+			clock-output-names = "clock_uart0";
+			clocks = <&clock_uart_p>;
+			clock-names = "clock_uart_p";
+		};
 	};
 };
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: apple: add uart gate clocks

[Stephen Boyd]

Quoting Sven Peter (2021-05-24 11:27:45)
> Now that we have a driver for gate clocks add the proper topology for the
> UART. These are already enabled by the bootloader but are part of the
> clock topology used by devices yet to be implemented.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)

Mostly an FYI; I will not pick up these dts patches in the clk tree. If
you can send these in a different series it makes my life easier because
then I can apply the whole thread without having to manually kick out
the dts bits from the mbox.

Re: [PATCH 3/3] arm64: apple: add uart gate clocks

[Stephen Boyd]

Quoting Sven Peter (2021-05-24 11:27:45)
> Now that we have a driver for gate clocks add the proper topology for the
> UART. These are already enabled by the bootloader but are part of the
> clock topology used by devices yet to be implemented.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)

Mostly an FYI; I will not pick up these dts patches in the clk tree. If
you can send these in a different series it makes my life easier because
then I can apply the whole thread without having to manually kick out
the dts bits from the mbox.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: apple: add uart gate clocks

[Sven Peter]

Hi,


On Wed, May 26, 2021, at 05:10, Stephen Boyd wrote:
> Quoting Sven Peter (2021-05-24 11:27:45)
> > Now that we have a driver for gate clocks add the proper topology for the
> > UART. These are already enabled by the bootloader but are part of the
> > clock topology used by devices yet to be implemented.
> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > ---
> >  arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> Mostly an FYI; I will not pick up these dts patches in the clk tree. If
> you can send these in a different series it makes my life easier because
> then I can apply the whole thread without having to manually kick out
> the dts bits from the mbox.
> 

Sure, I'll split the last patch from this series and send it as a one-off patch
once the rest has been reviewed and is acceptable :)

Thanks,

Sven

Re: [PATCH 3/3] arm64: apple: add uart gate clocks

[Sven Peter]

Hi,


On Wed, May 26, 2021, at 05:10, Stephen Boyd wrote:
> Quoting Sven Peter (2021-05-24 11:27:45)
> > Now that we have a driver for gate clocks add the proper topology for the
> > UART. These are already enabled by the bootloader but are part of the
> > clock topology used by devices yet to be implemented.
> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > ---
> >  arch/arm64/boot/dts/apple/t8103.dtsi | 36 +++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> Mostly an FYI; I will not pick up these dts patches in the clk tree. If
> you can send these in a different series it makes my life easier because
> then I can apply the whole thread without having to manually kick out
> the dts bits from the mbox.
> 

Sure, I'll split the last patch from this series and send it as a one-off patch
once the rest has been reviewed and is acceptable :)

Thanks,

Sven



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Rob Herring]

On Mon, May 24, 2021 at 1:28 PM Sven Peter <sven@svenpeter.dev> wrote:
>
>
> Hi,
>
> This series adds support for the clock gates present in Apple's SoCs such as
> the M1.
>
> These SoCs have various clock gates for their peripherals usually located in
> one or two MMIO regions. Each clock gate is a single 32 bit register which
> contains bits for the requested and the actual mode. The mode is represented by
> four bits. We however only care about "everything enabled" (0b1111) and
> "everything disabled" (0b000) for now. The other modes are probably different
> low-power states which don't even seem to be used by MacOS. The actual power
> management is located in a different region (and probably handled by a separate
> processor on the M1).
>
> Additionally, these clocks have a topology that is usually specified in Apple's
> Device Tree. As far as I can tell most Linux drivers seem to encode this topology
> directly in the source code though. Despite this, we would still like to encode
> the topology in the device tree itself:

We only define leaf clocks primarily. There's some exceptions such as
if PLLs are a separate h/w block. The reason for this is because
typical SoCs have 100s of just leaf clocks. If we tried to model
everything, it would never be complete nor correct. Actually, we did
try that at first.

> Apple seems to only change HW blocks when they have a very good reason and even
> then they seem to keep the changes minimal. This specific clock gate MMIO block
> has already been used in the Apple A7 released in 2013. The only differences
> since then are the available clocks (which can be identified by a simple offset)
> and their topology.

Clock gates are easy. What about muxes, dividers, etc.?

> It's likely that Apple will still use this block in future SoCs as well. By
> encoding the clock gate topology inside the device tree as well we can use a
> single driver and only need updates to the device tree once they are released.
> This might allow end users to already run their favorite distribution by just
> updating the bootloader with a new device tree instead of having to wait until
> the new topology is integrated into their distro kernel.
>
> Additionally, the driver itself can be kept very simple and not much code needs
> to be duplicated and then updated for each new SoC between various consumers of
> these device tree nodes (Linux, u-boot, and OpenBSD for now).
>
>
> This series therefore creates a single device tree node for each clock gate.
> This unfortunately maps a whole page out of which only a single register will
> be used for each node.
>
> The other alternative I considered was to create a syscon/simple-mfd node
> and have the clock nodes as children. The gates would then use a regmap which
> would only require a single entry in the pagetable for all clocks. I decided
> against this option since the clock gate MMIO region actually isn't a
> multi-function device.

I would do a single node per mmio region with the register offset (or
offset / 4) being the clock id. This can still support new SoCs easily
if you have a fallback compatible. If you want/need to get all the
clocks, just walk the DT 'clocks' properties and extract all the IDs.

Rob

Re: [PATCH 0/3] Apple M1 clock gate driver

[Rob Herring]

On Mon, May 24, 2021 at 1:28 PM Sven Peter <sven@svenpeter.dev> wrote:
>
>
> Hi,
>
> This series adds support for the clock gates present in Apple's SoCs such as
> the M1.
>
> These SoCs have various clock gates for their peripherals usually located in
> one or two MMIO regions. Each clock gate is a single 32 bit register which
> contains bits for the requested and the actual mode. The mode is represented by
> four bits. We however only care about "everything enabled" (0b1111) and
> "everything disabled" (0b000) for now. The other modes are probably different
> low-power states which don't even seem to be used by MacOS. The actual power
> management is located in a different region (and probably handled by a separate
> processor on the M1).
>
> Additionally, these clocks have a topology that is usually specified in Apple's
> Device Tree. As far as I can tell most Linux drivers seem to encode this topology
> directly in the source code though. Despite this, we would still like to encode
> the topology in the device tree itself:

We only define leaf clocks primarily. There's some exceptions such as
if PLLs are a separate h/w block. The reason for this is because
typical SoCs have 100s of just leaf clocks. If we tried to model
everything, it would never be complete nor correct. Actually, we did
try that at first.

> Apple seems to only change HW blocks when they have a very good reason and even
> then they seem to keep the changes minimal. This specific clock gate MMIO block
> has already been used in the Apple A7 released in 2013. The only differences
> since then are the available clocks (which can be identified by a simple offset)
> and their topology.

Clock gates are easy. What about muxes, dividers, etc.?

> It's likely that Apple will still use this block in future SoCs as well. By
> encoding the clock gate topology inside the device tree as well we can use a
> single driver and only need updates to the device tree once they are released.
> This might allow end users to already run their favorite distribution by just
> updating the bootloader with a new device tree instead of having to wait until
> the new topology is integrated into their distro kernel.
>
> Additionally, the driver itself can be kept very simple and not much code needs
> to be duplicated and then updated for each new SoC between various consumers of
> these device tree nodes (Linux, u-boot, and OpenBSD for now).
>
>
> This series therefore creates a single device tree node for each clock gate.
> This unfortunately maps a whole page out of which only a single register will
> be used for each node.
>
> The other alternative I considered was to create a syscon/simple-mfd node
> and have the clock nodes as children. The gates would then use a regmap which
> would only require a single entry in the pagetable for all clocks. I decided
> against this option since the clock gate MMIO region actually isn't a
> multi-function device.

I would do a single node per mmio region with the register offset (or
offset / 4) being the clock id. This can still support new SoCs easily
if you have a fallback compatible. If you want/need to get all the
clocks, just walk the DT 'clocks' properties and extract all the IDs.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

Hi,

* Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> I would do a single node per mmio region with the register offset (or
> offset / 4) being the clock id. This can still support new SoCs easily
> if you have a fallback compatible. If you want/need to get all the
> clocks, just walk the DT 'clocks' properties and extract all the IDs.

I mostly agree.. Except I'd also leave out the artificial clock ID and
just use real register offsets from the clock controller base instead.

So a single clock controller node for each MMIO range, then set
#clock=cells = <1>. Then the binding follows what we have for the
interrupts-extended binding for example.

If the clock controller optionally needs some data in the dts,
that can be added to the clock controller node. Or it can be driver
internal built-in data. If the data for dts can be described in a
generic way, even better :)

This would make the consumer interface look like below with a
clock controller node and register offset from it:

clocks = <&clock_controller1 0x1234>;

Regards,

Tony

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

Hi,

* Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> I would do a single node per mmio region with the register offset (or
> offset / 4) being the clock id. This can still support new SoCs easily
> if you have a fallback compatible. If you want/need to get all the
> clocks, just walk the DT 'clocks' properties and extract all the IDs.

I mostly agree.. Except I'd also leave out the artificial clock ID and
just use real register offsets from the clock controller base instead.

So a single clock controller node for each MMIO range, then set
#clock=cells = <1>. Then the binding follows what we have for the
interrupts-extended binding for example.

If the clock controller optionally needs some data in the dts,
that can be added to the clock controller node. Or it can be driver
internal built-in data. If the data for dts can be described in a
generic way, even better :)

This would make the consumer interface look like below with a
clock controller node and register offset from it:

clocks = <&clock_controller1 0x1234>;

Regards,

Tony




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi,

On Wed, May 26, 2021, at 09:18, Tony Lindgren wrote:
> Hi,
> 
> * Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> > I would do a single node per mmio region with the register offset (or
> > offset / 4) being the clock id. This can still support new SoCs easily
> > if you have a fallback compatible. If you want/need to get all the
> > clocks, just walk the DT 'clocks' properties and extract all the IDs.
> 
> I mostly agree.. Except I'd also leave out the artificial clock ID and
> just use real register offsets from the clock controller base instead.

Sure, I'll do that.

> 
> So a single clock controller node for each MMIO range, then set
> #clock=cells = <1>. Then the binding follows what we have for the
> interrupts-extended binding for example.
> 
> If the clock controller optionally needs some data in the dts,
> that can be added to the clock controller node. Or it can be driver
> internal built-in data. If the data for dts can be described in a
> generic way, even better :)

Now the big question is *how* to describe this additional data in the
dts. Essentially I need to specify that e.g. to enable clock 0x270
I first need to enable the (internal) clocks 0x1c0 and then 0x220.
Are you aware of any generic way to describe this? I'm not even sure
how a sane non-generic way would look like when I just have a single
clock controller node.



Best,

Sven

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi,

On Wed, May 26, 2021, at 09:18, Tony Lindgren wrote:
> Hi,
> 
> * Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> > I would do a single node per mmio region with the register offset (or
> > offset / 4) being the clock id. This can still support new SoCs easily
> > if you have a fallback compatible. If you want/need to get all the
> > clocks, just walk the DT 'clocks' properties and extract all the IDs.
> 
> I mostly agree.. Except I'd also leave out the artificial clock ID and
> just use real register offsets from the clock controller base instead.

Sure, I'll do that.

> 
> So a single clock controller node for each MMIO range, then set
> #clock=cells = <1>. Then the binding follows what we have for the
> interrupts-extended binding for example.
> 
> If the clock controller optionally needs some data in the dts,
> that can be added to the clock controller node. Or it can be driver
> internal built-in data. If the data for dts can be described in a
> generic way, even better :)

Now the big question is *how* to describe this additional data in the
dts. Essentially I need to specify that e.g. to enable clock 0x270
I first need to enable the (internal) clocks 0x1c0 and then 0x220.
Are you aware of any generic way to describe this? I'm not even sure
how a sane non-generic way would look like when I just have a single
clock controller node.



Best,

Sven

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210530 11:09]:
> Hi,
> 
> On Wed, May 26, 2021, at 09:18, Tony Lindgren wrote:
> > Hi,
> > 
> > * Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> > > I would do a single node per mmio region with the register offset (or
> > > offset / 4) being the clock id. This can still support new SoCs easily
> > > if you have a fallback compatible. If you want/need to get all the
> > > clocks, just walk the DT 'clocks' properties and extract all the IDs.
> > 
> > I mostly agree.. Except I'd also leave out the artificial clock ID and
> > just use real register offsets from the clock controller base instead.
> 
> Sure, I'll do that.
> 
> > 
> > So a single clock controller node for each MMIO range, then set
> > #clock=cells = <1>. Then the binding follows what we have for the
> > interrupts-extended binding for example.
> > 
> > If the clock controller optionally needs some data in the dts,
> > that can be added to the clock controller node. Or it can be driver
> > internal built-in data. If the data for dts can be described in a
> > generic way, even better :)
> 
> Now the big question is *how* to describe this additional data in the
> dts. Essentially I need to specify that e.g. to enable clock 0x270
> I first need to enable the (internal) clocks 0x1c0 and then 0x220.
> Are you aware of any generic way to describe this? I'm not even sure
> how a sane non-generic way would look like when I just have a single
> clock controller node.

To me it seems you might be able to recycle the assigned-clocks and
assigned-clock-parents etc properties in the clock controller node.

Sure the assigned-clocks property will point to clocks in the
clock controller itself, and will have tens of entries, but should
work :)

And sounds like you can generate that list with some script from the
Apple dtb.

Regards,

Tony

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210530 11:09]:
> Hi,
> 
> On Wed, May 26, 2021, at 09:18, Tony Lindgren wrote:
> > Hi,
> > 
> > * Rob Herring <robh+dt@kernel.org> [210525 18:09]:
> > > I would do a single node per mmio region with the register offset (or
> > > offset / 4) being the clock id. This can still support new SoCs easily
> > > if you have a fallback compatible. If you want/need to get all the
> > > clocks, just walk the DT 'clocks' properties and extract all the IDs.
> > 
> > I mostly agree.. Except I'd also leave out the artificial clock ID and
> > just use real register offsets from the clock controller base instead.
> 
> Sure, I'll do that.
> 
> > 
> > So a single clock controller node for each MMIO range, then set
> > #clock=cells = <1>. Then the binding follows what we have for the
> > interrupts-extended binding for example.
> > 
> > If the clock controller optionally needs some data in the dts,
> > that can be added to the clock controller node. Or it can be driver
> > internal built-in data. If the data for dts can be described in a
> > generic way, even better :)
> 
> Now the big question is *how* to describe this additional data in the
> dts. Essentially I need to specify that e.g. to enable clock 0x270
> I first need to enable the (internal) clocks 0x1c0 and then 0x220.
> Are you aware of any generic way to describe this? I'm not even sure
> how a sane non-generic way would look like when I just have a single
> clock controller node.

To me it seems you might be able to recycle the assigned-clocks and
assigned-clock-parents etc properties in the clock controller node.

Sure the assigned-clocks property will point to clocks in the
clock controller itself, and will have tens of entries, but should
work :)

And sounds like you can generate that list with some script from the
Apple dtb.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi Tony,

On Wed, Jun 2, 2021, at 11:26, Tony Lindgren wrote:
> * Sven Peter <sven@svenpeter.dev> [210530 11:09]:
> > 
> > Now the big question is *how* to describe this additional data in the
> > dts. Essentially I need to specify that e.g. to enable clock 0x270
> > I first need to enable the (internal) clocks 0x1c0 and then 0x220.
> > Are you aware of any generic way to describe this? I'm not even sure
> > how a sane non-generic way would look like when I just have a single
> > clock controller node.
> 
> To me it seems you might be able to recycle the assigned-clocks and
> assigned-clock-parents etc properties in the clock controller node.
> 
> Sure the assigned-clocks property will point to clocks in the
> clock controller itself, and will have tens of entries, but should
> work :)

Thanks for the suggestion, I really like that idea!


If I understand the assigned-clocks-parent property correctly it's meant to select
one of the possible parents of e.g. a mux clock at startup. Internally it just
uses clk_set_parent which, unless I'm mistaken, would require to assign each
clock as a possible parent of every other clock.

Now the good news is that Apple seems to have sorted the clocks topologically
on the bus, i.e. there never is a clock at address X that requires a parent
with an address bigger than X. 
The bad news is that I'd probably still have to setup clocks at 0x0, 0x4,
0x8, ..., X-0x4 as possible parents of the clock at X.

Another possibility this made me think of is to instead just use the clocks
property the way it's usually used and simply refer to the controller itself, e.g.

#define APPLE_CLK_UART0  0x270
#define APPLE_CLK_UART_P 0x220
#define APPLE_CLK_SIO    0x1c0

pmgr0: clock-controller@23b700000 {
        compatible = "apple,t8103-gate-clock";
        #clock-cells = <1>;
        reg = <0x2 0x3b700000 0x0 0x4000>;
        clock-indices = <APPLE_CLK_SIO>, <APPLE_CLK_UART_P>, <APPLE_CLK_UART0>;
        clock-output-names = "clock-sio", "clock-uart-", "clock-uart0";
        clocks = <&some_dummy_root_clock>, <&pmgr0 APPLE_CLK_SIO>,
                 <&pmgr0 APPLE_CLK_UART_P>;
};

to represent the UART clocks

    UART0  (0x23b700270), parent: UART_P
    UART_P (0x23b700220), parent: SIO
    SIO    (0x23b7001c0), parent: n/a

and then have the consumer as

serial0: serial@235200000 {
    // ...
    clocks = <&pmgr0 APPLE_CLK_UART0>, <&clk24>;
    clock-names = "uart", "clk_uart_baud0";
    // ...
};


I'd have to see if it's possible to implement this sanely though if this binding
was acceptable. The self-reference to a node that hasn't been fully initialized
yet may turn out to be ugly.


> 
> And sounds like you can generate that list with some script from the
> Apple dtb.

Yup, absolutely. I don't want to write this all out by hand if I can avoid
that :-)

> 
> Regards,
> 
> Tony
> 

Thanks,


Sven

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi Tony,

On Wed, Jun 2, 2021, at 11:26, Tony Lindgren wrote:
> * Sven Peter <sven@svenpeter.dev> [210530 11:09]:
> > 
> > Now the big question is *how* to describe this additional data in the
> > dts. Essentially I need to specify that e.g. to enable clock 0x270
> > I first need to enable the (internal) clocks 0x1c0 and then 0x220.
> > Are you aware of any generic way to describe this? I'm not even sure
> > how a sane non-generic way would look like when I just have a single
> > clock controller node.
> 
> To me it seems you might be able to recycle the assigned-clocks and
> assigned-clock-parents etc properties in the clock controller node.
> 
> Sure the assigned-clocks property will point to clocks in the
> clock controller itself, and will have tens of entries, but should
> work :)

Thanks for the suggestion, I really like that idea!


If I understand the assigned-clocks-parent property correctly it's meant to select
one of the possible parents of e.g. a mux clock at startup. Internally it just
uses clk_set_parent which, unless I'm mistaken, would require to assign each
clock as a possible parent of every other clock.

Now the good news is that Apple seems to have sorted the clocks topologically
on the bus, i.e. there never is a clock at address X that requires a parent
with an address bigger than X. 
The bad news is that I'd probably still have to setup clocks at 0x0, 0x4,
0x8, ..., X-0x4 as possible parents of the clock at X.

Another possibility this made me think of is to instead just use the clocks
property the way it's usually used and simply refer to the controller itself, e.g.

#define APPLE_CLK_UART0  0x270
#define APPLE_CLK_UART_P 0x220
#define APPLE_CLK_SIO    0x1c0

pmgr0: clock-controller@23b700000 {
        compatible = "apple,t8103-gate-clock";
        #clock-cells = <1>;
        reg = <0x2 0x3b700000 0x0 0x4000>;
        clock-indices = <APPLE_CLK_SIO>, <APPLE_CLK_UART_P>, <APPLE_CLK_UART0>;
        clock-output-names = "clock-sio", "clock-uart-", "clock-uart0";
        clocks = <&some_dummy_root_clock>, <&pmgr0 APPLE_CLK_SIO>,
                 <&pmgr0 APPLE_CLK_UART_P>;
};

to represent the UART clocks

    UART0  (0x23b700270), parent: UART_P
    UART_P (0x23b700220), parent: SIO
    SIO    (0x23b7001c0), parent: n/a

and then have the consumer as

serial0: serial@235200000 {
    // ...
    clocks = <&pmgr0 APPLE_CLK_UART0>, <&clk24>;
    clock-names = "uart", "clk_uart_baud0";
    // ...
};


I'd have to see if it's possible to implement this sanely though if this binding
was acceptable. The self-reference to a node that hasn't been fully initialized
yet may turn out to be ugly.


> 
> And sounds like you can generate that list with some script from the
> Apple dtb.

Yup, absolutely. I don't want to write this all out by hand if I can avoid
that :-)

> 
> Regards,
> 
> Tony
> 

Thanks,


Sven


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

Hi,

* Sven Peter <sven@svenpeter.dev> [210603 12:56]:
> Another possibility this made me think of is to instead just use the clocks
> property the way it's usually used and simply refer to the controller itself, e.g.
> 
> #define APPLE_CLK_UART0  0x270
> #define APPLE_CLK_UART_P 0x220
> #define APPLE_CLK_SIO    0x1c0
> 
> pmgr0: clock-controller@23b700000 {
>         compatible = "apple,t8103-gate-clock";
>         #clock-cells = <1>;
>         reg = <0x2 0x3b700000 0x0 0x4000>;
>         clock-indices = <APPLE_CLK_SIO>, <APPLE_CLK_UART_P>, <APPLE_CLK_UART0>;
>         clock-output-names = "clock-sio", "clock-uart-", "clock-uart0";
>         clocks = <&some_dummy_root_clock>, <&pmgr0 APPLE_CLK_SIO>,
>                  <&pmgr0 APPLE_CLK_UART_P>;
> };

How about the following where you set up the gate clocks as separate child nodes:

pmgr0: clock-controller@23b700000 {
	compatible = "apple,foo-clock-controller";
	#clock-cells = <1>;
	reg = <0x2 0x3b700000 0x0 0x4000>;

	clk_uart0: clock@270 {
		compatible = "apple,t8103-gate-clock";
		#clock-cells = <0>;
		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
					 <&pmgr0 APPLE_CLK_UART_P>;
		// ...
	};

};

Keep the clock controller still addressable by offset from base as discussed,
and additionally have the driver parse and set up the child node clocks.

Then I think the consumer driver can just do:

serial0: serial@235200000 {
	// ...
	clocks = <&clk_uart0>, <&clk24>;
	clock-names = "uart", "clk_uart_baud0";
	// ...
};

Regards,

Tony

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

Hi,

* Sven Peter <sven@svenpeter.dev> [210603 12:56]:
> Another possibility this made me think of is to instead just use the clocks
> property the way it's usually used and simply refer to the controller itself, e.g.
> 
> #define APPLE_CLK_UART0  0x270
> #define APPLE_CLK_UART_P 0x220
> #define APPLE_CLK_SIO    0x1c0
> 
> pmgr0: clock-controller@23b700000 {
>         compatible = "apple,t8103-gate-clock";
>         #clock-cells = <1>;
>         reg = <0x2 0x3b700000 0x0 0x4000>;
>         clock-indices = <APPLE_CLK_SIO>, <APPLE_CLK_UART_P>, <APPLE_CLK_UART0>;
>         clock-output-names = "clock-sio", "clock-uart-", "clock-uart0";
>         clocks = <&some_dummy_root_clock>, <&pmgr0 APPLE_CLK_SIO>,
>                  <&pmgr0 APPLE_CLK_UART_P>;
> };

How about the following where you set up the gate clocks as separate child nodes:

pmgr0: clock-controller@23b700000 {
	compatible = "apple,foo-clock-controller";
	#clock-cells = <1>;
	reg = <0x2 0x3b700000 0x0 0x4000>;

	clk_uart0: clock@270 {
		compatible = "apple,t8103-gate-clock";
		#clock-cells = <0>;
		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
					 <&pmgr0 APPLE_CLK_UART_P>;
		// ...
	};

};

Keep the clock controller still addressable by offset from base as discussed,
and additionally have the driver parse and set up the child node clocks.

Then I think the consumer driver can just do:

serial0: serial@235200000 {
	// ...
	clocks = <&clk_uart0>, <&clk24>;
	clock-names = "uart", "clk_uart_baud0";
	// ...
};

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi Tony,

On Fri, Jun 4, 2021, at 09:43, Tony Lindgren wrote:
> Hi,
> 
> How about the following where you set up the gate clocks as separate 
> child nodes:
> 
> pmgr0: clock-controller@23b700000 {
> 	compatible = "apple,foo-clock-controller";
> 	#clock-cells = <1>;
> 	reg = <0x2 0x3b700000 0x0 0x4000>;
> 
> 	clk_uart0: clock@270 {
> 		compatible = "apple,t8103-gate-clock";
> 		#clock-cells = <0>;
> 		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
> 					 <&pmgr0 APPLE_CLK_UART_P>;
> 		// ...
> 	};
> 
> };
> 
> Keep the clock controller still addressable by offset from base as discussed,
> and additionally have the driver parse and set up the child node clocks.

Nice, I like that one even better! We can keep the internal clocks "hidden"
inside the parent node and only need to model the actual consumer clocks
as separate nodes.

Are you aware of any clock driver that implements something similar?
I'd like to avoid reinventing the wheel if it's already been done before.

> 
> Then I think the consumer driver can just do:
> 
> serial0: serial@235200000 {
> 	// ...
> 	clocks = <&clk_uart0>, <&clk24>;
> 	clock-names = "uart", "clk_uart_baud0";
> 	// ...
> };
> 
> Regards,
> 
> Tony
> 

Best,

Sven

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

Hi Tony,

On Fri, Jun 4, 2021, at 09:43, Tony Lindgren wrote:
> Hi,
> 
> How about the following where you set up the gate clocks as separate 
> child nodes:
> 
> pmgr0: clock-controller@23b700000 {
> 	compatible = "apple,foo-clock-controller";
> 	#clock-cells = <1>;
> 	reg = <0x2 0x3b700000 0x0 0x4000>;
> 
> 	clk_uart0: clock@270 {
> 		compatible = "apple,t8103-gate-clock";
> 		#clock-cells = <0>;
> 		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
> 					 <&pmgr0 APPLE_CLK_UART_P>;
> 		// ...
> 	};
> 
> };
> 
> Keep the clock controller still addressable by offset from base as discussed,
> and additionally have the driver parse and set up the child node clocks.

Nice, I like that one even better! We can keep the internal clocks "hidden"
inside the parent node and only need to model the actual consumer clocks
as separate nodes.

Are you aware of any clock driver that implements something similar?
I'd like to avoid reinventing the wheel if it's already been done before.

> 
> Then I think the consumer driver can just do:
> 
> serial0: serial@235200000 {
> 	// ...
> 	clocks = <&clk_uart0>, <&clk24>;
> 	clock-names = "uart", "clk_uart_baud0";
> 	// ...
> };
> 
> Regards,
> 
> Tony
> 

Best,

Sven

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210605 12:13]:
> Hi Tony,
> 
> On Fri, Jun 4, 2021, at 09:43, Tony Lindgren wrote:
> > Hi,
> > 
> > How about the following where you set up the gate clocks as separate 
> > child nodes:
> > 
> > pmgr0: clock-controller@23b700000 {
> > 	compatible = "apple,foo-clock-controller";
> > 	#clock-cells = <1>;
> > 	reg = <0x2 0x3b700000 0x0 0x4000>;
> > 
> > 	clk_uart0: clock@270 {
> > 		compatible = "apple,t8103-gate-clock";
> > 		#clock-cells = <0>;
> > 		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
> > 					 <&pmgr0 APPLE_CLK_UART_P>;
> > 		// ...
> > 	};
> > 
> > };
> > 
> > Keep the clock controller still addressable by offset from base as discussed,
> > and additionally have the driver parse and set up the child node clocks.
>
> Nice, I like that one even better! We can keep the internal clocks "hidden"
> inside the parent node and only need to model the actual consumer clocks
> as separate nodes.

I guess the child nodes could also use just a clocks property above
instead of assigned-clock related properties if there are no configurable
source clock mux registers.

> Are you aware of any clock driver that implements something similar?
> I'd like to avoid reinventing the wheel if it's already been done before.

I'm only aware of a partial implementation so far :) The "offset from
clock controller base" approach has worked well for the TI clkctrl
clocks. The clkctrl gate clocks still have the SoC specific source
clock data build into the clock driver(s).

That's the Documentation/devicetree/bindings/clock/ti-clkctrl.txt
binding. For the clkctrl clocks, the SoC specific source clock data
is in drivers/clk/ti/clk-*.c files.

With the Apple dtb describing the gate clock parents, you might be
able to leave out most of the SoC specific built-in driver data
sounds like.

Regards,

Tony

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210605 12:13]:
> Hi Tony,
> 
> On Fri, Jun 4, 2021, at 09:43, Tony Lindgren wrote:
> > Hi,
> > 
> > How about the following where you set up the gate clocks as separate 
> > child nodes:
> > 
> > pmgr0: clock-controller@23b700000 {
> > 	compatible = "apple,foo-clock-controller";
> > 	#clock-cells = <1>;
> > 	reg = <0x2 0x3b700000 0x0 0x4000>;
> > 
> > 	clk_uart0: clock@270 {
> > 		compatible = "apple,t8103-gate-clock";
> > 		#clock-cells = <0>;
> > 		assigned-clock-parents = <&pmgr0 APPLE_CLK_SIO>,
> > 					 <&pmgr0 APPLE_CLK_UART_P>;
> > 		// ...
> > 	};
> > 
> > };
> > 
> > Keep the clock controller still addressable by offset from base as discussed,
> > and additionally have the driver parse and set up the child node clocks.
>
> Nice, I like that one even better! We can keep the internal clocks "hidden"
> inside the parent node and only need to model the actual consumer clocks
> as separate nodes.

I guess the child nodes could also use just a clocks property above
instead of assigned-clock related properties if there are no configurable
source clock mux registers.

> Are you aware of any clock driver that implements something similar?
> I'd like to avoid reinventing the wheel if it's already been done before.

I'm only aware of a partial implementation so far :) The "offset from
clock controller base" approach has worked well for the TI clkctrl
clocks. The clkctrl gate clocks still have the SoC specific source
clock data build into the clock driver(s).

That's the Documentation/devicetree/bindings/clock/ti-clkctrl.txt
binding. For the clkctrl clocks, the SoC specific source clock data
is in drivers/clk/ti/clk-*.c files.

With the Apple dtb describing the gate clock parents, you might be
able to leave out most of the SoC specific built-in driver data
sounds like.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

On Tue, May 25, 2021, at 19:41, Rob Herring wrote:
> On Mon, May 24, 2021 at 1:28 PM Sven Peter <sven@svenpeter.dev> wrote:
> >
> > Additionally, these clocks have a topology that is usually specified in Apple's
> > Device Tree. As far as I can tell most Linux drivers seem to encode this topology
> > directly in the source code though. Despite this, we would still like to encode
> > the topology in the device tree itself:
> 
> We only define leaf clocks primarily. There's some exceptions such as
> if PLLs are a separate h/w block. The reason for this is because
> typical SoCs have 100s of just leaf clocks. If we tried to model
> everything, it would never be complete nor correct. Actually, we did
> try that at first.

Makes sense.

For Apple SoCs specifically most topology seems to documented (with names!)
in the arm-io/pmgr device node of their ADT.

> 
> > Apple seems to only change HW blocks when they have a very good reason and even
> > then they seem to keep the changes minimal. This specific clock gate MMIO block
> > has already been used in the Apple A7 released in 2013. The only differences
> > since then are the available clocks (which can be identified by a simple offset)
> > and their topology.
> 
> Clock gates are easy. What about muxes, dividers, etc.?

These are preconfigured by iBoot long before we get control and I'm not
sure we will be able to figure out how they work in detail. All we know
are their frequencies and we will probably have to just treat them as
fixed frequency clocks.

Some details:

In Apple's device tree, each node can have a "clock-gates" or "power-gates"
property which refer to clock gates required to enable the peripheral.
These are implemented with this small driver and required to make most of
the hardware work. Most of these are turned off by the time we get control.

Additionally, there is a "clocks" property that either refers to one of
eight or so static clocks (starting at index 0) which are not backed by any
configurable PLL, mux or divider. On top of that there is a list of what I
believe to be mux/divider clocks (index >0x100) specified by the
clock-frequencies and clock-frequencies-regs properties of the arm-io node.
These registers are in a separate MMIO region and XNU only seems to use
clock-frequencies to find the clock rate. It never even reads
clock-frequencies-regs.

While I can guess some of the bits of these registers (I believe I know the
enable bit and the divisor field and possibly the input select but I do not
know the inputs and believe they are actually different for each clock) I
don't see a need to implement them right now. If we need them in the future
they'd probably be a separate node anyway since they are all also located in
an entirely different MMIO region and not related to these gates at all.


> 
> > It's likely that Apple will still use this block in future SoCs as well. By
> > encoding the clock gate topology inside the device tree as well we can use a
> > single driver and only need updates to the device tree once they are released.
> > This might allow end users to already run their favorite distribution by just
> > updating the bootloader with a new device tree instead of having to wait until
> > the new topology is integrated into their distro kernel.
> >
> > Additionally, the driver itself can be kept very simple and not much code needs
> > to be duplicated and then updated for each new SoC between various consumers of
> > these device tree nodes (Linux, u-boot, and OpenBSD for now).
> >
> >
> > This series therefore creates a single device tree node for each clock gate.
> > This unfortunately maps a whole page out of which only a single register will
> > be used for each node.
> >
> > The other alternative I considered was to create a syscon/simple-mfd node
> > and have the clock nodes as children. The gates would then use a regmap which
> > would only require a single entry in the pagetable for all clocks. I decided
> > against this option since the clock gate MMIO region actually isn't a
> > multi-function device.
> 
> I would do a single node per mmio region with the register offset (or
> offset / 4) being the clock id. This can still support new SoCs easily
> if you have a fallback compatible. If you want/need to get all the
> clocks, just walk the DT 'clocks' properties and extract all the IDs.
> 

The problem with that approach is that to enable e.g. UART_0 we actually need
to enable its parents as well, e.g. the Apple Device Tree for the M1 has the
following clock topology:

    UART0  (0x23b700270), parent: UART_P
    UART_P (0x23b700220), parent: SIO
    SIO    (0x23b7001c0), parent: n/a

The offsets and the parent/child relationship for all of these three clocks
change between SoCs. If I now use the offset as the clock id I still need
to specify that if e.g. UART uses <&clk_controller 0x270> I first need
to enable 0x1c0 and then 0x220 and only then 0x270.

I can hardcode this in the driver itself by just selecting the appropriate table
depending on the compatible but I think we're then back to requiring a code
change for new SoCs. Just using the fallback compatible won't be enough unless
I introduce dummy nodes that enable UART_P and SIO as well.

If I do it like this I'll also need two compatibles for the two MMIO regions
since they have different child/parent clocks in there. Any suggestions for
how to call those two then? Just t8103-gate-clock-0/1? 

I'd still prefer to somehow put this information into the device tree since
then only a single compatible that will likely work across many SoC generations
is required. I'm not sure how to do this though.


Best,


Sven

Re: [PATCH 0/3] Apple M1 clock gate driver

[Sven Peter]

On Tue, May 25, 2021, at 19:41, Rob Herring wrote:
> On Mon, May 24, 2021 at 1:28 PM Sven Peter <sven@svenpeter.dev> wrote:
> >
> > Additionally, these clocks have a topology that is usually specified in Apple's
> > Device Tree. As far as I can tell most Linux drivers seem to encode this topology
> > directly in the source code though. Despite this, we would still like to encode
> > the topology in the device tree itself:
> 
> We only define leaf clocks primarily. There's some exceptions such as
> if PLLs are a separate h/w block. The reason for this is because
> typical SoCs have 100s of just leaf clocks. If we tried to model
> everything, it would never be complete nor correct. Actually, we did
> try that at first.

Makes sense.

For Apple SoCs specifically most topology seems to documented (with names!)
in the arm-io/pmgr device node of their ADT.

> 
> > Apple seems to only change HW blocks when they have a very good reason and even
> > then they seem to keep the changes minimal. This specific clock gate MMIO block
> > has already been used in the Apple A7 released in 2013. The only differences
> > since then are the available clocks (which can be identified by a simple offset)
> > and their topology.
> 
> Clock gates are easy. What about muxes, dividers, etc.?

These are preconfigured by iBoot long before we get control and I'm not
sure we will be able to figure out how they work in detail. All we know
are their frequencies and we will probably have to just treat them as
fixed frequency clocks.

Some details:

In Apple's device tree, each node can have a "clock-gates" or "power-gates"
property which refer to clock gates required to enable the peripheral.
These are implemented with this small driver and required to make most of
the hardware work. Most of these are turned off by the time we get control.

Additionally, there is a "clocks" property that either refers to one of
eight or so static clocks (starting at index 0) which are not backed by any
configurable PLL, mux or divider. On top of that there is a list of what I
believe to be mux/divider clocks (index >0x100) specified by the
clock-frequencies and clock-frequencies-regs properties of the arm-io node.
These registers are in a separate MMIO region and XNU only seems to use
clock-frequencies to find the clock rate. It never even reads
clock-frequencies-regs.

While I can guess some of the bits of these registers (I believe I know the
enable bit and the divisor field and possibly the input select but I do not
know the inputs and believe they are actually different for each clock) I
don't see a need to implement them right now. If we need them in the future
they'd probably be a separate node anyway since they are all also located in
an entirely different MMIO region and not related to these gates at all.


> 
> > It's likely that Apple will still use this block in future SoCs as well. By
> > encoding the clock gate topology inside the device tree as well we can use a
> > single driver and only need updates to the device tree once they are released.
> > This might allow end users to already run their favorite distribution by just
> > updating the bootloader with a new device tree instead of having to wait until
> > the new topology is integrated into their distro kernel.
> >
> > Additionally, the driver itself can be kept very simple and not much code needs
> > to be duplicated and then updated for each new SoC between various consumers of
> > these device tree nodes (Linux, u-boot, and OpenBSD for now).
> >
> >
> > This series therefore creates a single device tree node for each clock gate.
> > This unfortunately maps a whole page out of which only a single register will
> > be used for each node.
> >
> > The other alternative I considered was to create a syscon/simple-mfd node
> > and have the clock nodes as children. The gates would then use a regmap which
> > would only require a single entry in the pagetable for all clocks. I decided
> > against this option since the clock gate MMIO region actually isn't a
> > multi-function device.
> 
> I would do a single node per mmio region with the register offset (or
> offset / 4) being the clock id. This can still support new SoCs easily
> if you have a fallback compatible. If you want/need to get all the
> clocks, just walk the DT 'clocks' properties and extract all the IDs.
> 

The problem with that approach is that to enable e.g. UART_0 we actually need
to enable its parents as well, e.g. the Apple Device Tree for the M1 has the
following clock topology:

    UART0  (0x23b700270), parent: UART_P
    UART_P (0x23b700220), parent: SIO
    SIO    (0x23b7001c0), parent: n/a

The offsets and the parent/child relationship for all of these three clocks
change between SoCs. If I now use the offset as the clock id I still need
to specify that if e.g. UART uses <&clk_controller 0x270> I first need
to enable 0x1c0 and then 0x220 and only then 0x270.

I can hardcode this in the driver itself by just selecting the appropriate table
depending on the compatible but I think we're then back to requiring a code
change for new SoCs. Just using the fallback compatible won't be enough unless
I introduce dummy nodes that enable UART_P and SIO as well.

If I do it like this I'll also need two compatibles for the two MMIO regions
since they have different child/parent clocks in there. Any suggestions for
how to call those two then? Just t8103-gate-clock-0/1? 

I'd still prefer to somehow put this information into the device tree since
then only a single compatible that will likely work across many SoC generations
is required. I'm not sure how to do this though.


Best,


Sven

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210530 11:11]:
> The problem with that approach is that to enable e.g. UART_0 we actually need
> to enable its parents as well, e.g. the Apple Device Tree for the M1 has the
> following clock topology:
> 
>     UART0  (0x23b700270), parent: UART_P
>     UART_P (0x23b700220), parent: SIO
>     SIO    (0x23b7001c0), parent: n/a
> 
> The offsets and the parent/child relationship for all of these three clocks
> change between SoCs. If I now use the offset as the clock id I still need
> to specify that if e.g. UART uses <&clk_controller 0x270> I first need
> to enable 0x1c0 and then 0x220 and only then 0x270.

Maybe take a look what I suggested on using assigned-clocks and related
properties in the clock controller node. That might solve the issue in
a generic way for other SoCs too.

Regards,

Tony

Re: [PATCH 0/3] Apple M1 clock gate driver

[Tony Lindgren]

* Sven Peter <sven@svenpeter.dev> [210530 11:11]:
> The problem with that approach is that to enable e.g. UART_0 we actually need
> to enable its parents as well, e.g. the Apple Device Tree for the M1 has the
> following clock topology:
> 
>     UART0  (0x23b700270), parent: UART_P
>     UART_P (0x23b700220), parent: SIO
>     SIO    (0x23b7001c0), parent: n/a
> 
> The offsets and the parent/child relationship for all of these three clocks
> change between SoCs. If I now use the offset as the clock id I still need
> to specify that if e.g. UART uses <&clk_controller 0x270> I first need
> to enable 0x1c0 and then 0x220 and only then 0x270.

Maybe take a look what I suggested on using assigned-clocks and related
properties in the clock controller node. That might solve the issue in
a generic way for other SoCs too.

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel