xen/arm: register clocks used by the hypervisor

Quoted 41 lines · Dirk Behme

Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644
@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 29

Quoted 10 lines

Documentation/devicetree/bindings/arm/xen.txt +10 -0

+Optional properties:
+
+- clocks: one or more clocks to be registered.
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.

This shouldn't be described in terms of the Linux implementation details like clk_disable_unused. Those can change at any time, and don't define the contract as such.

I suspect it may be necessary to do more to ensure that, but I'm not familiar enough with the clocks subsystem to say.

Thanks,
Mark.

Quoted 72 lines

Documentation/devicetree/bindings/arm/xen.txt +1 -0

+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:

arch/arm/xen/enlighten.c +47 -0

index 47acb36..5c546d0 100644
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+	struct clk *clk;
+	struct device_node *xen_node;
+	unsigned int i, count;
+	int ret = 0;
+
+	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+	if (!xen_node) {
+		pr_err("Xen support was detected before, but it has disappeared\n");
+		return -EINVAL;
+	}
+
+	count = of_clk_get_parent_count(xen_node);
+	if (!count)
+		goto out;
+
+	for (i = 0; i < count; i++) {
+		clk = of_clk_get(xen_node, i);
+		if (IS_ERR(clk)) {
+			pr_err("Xen failed to register clock %i. Error: %li\n",
+			       i, PTR_ERR(clk));
+			ret = PTR_ERR(clk);
+			goto out;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret < 0) {
+			pr_err("Xen failed to enable clock %i. Error: %i\n",
+			       i, ret);
+			goto out;
+		}
+	}
+
+	ret = 0;
+
+out:
+	of_node_put(xen_node);
+	return ret;
+}
+late_initcall(xen_arm_register_clks);
 
 /* empty stubs */
 void xen_arch_pre_suspend(void) { }

Signature · 1 line

2.8.0

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Stefano Stabellini

maintainer

2016-07-05 13:53 UTC #8 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 41 lines · Dirk Behme

Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644
@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 29

Quoted 10 lines

Documentation/devicetree/bindings/arm/xen.txt +10 -0

+Optional properties:
+
+- clocks: one or more clocks to be registered.
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.

What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

Quoted 71 lines

Documentation/devicetree/bindings/arm/xen.txt +0 -0

 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:

arch/arm/xen/enlighten.c +47 -0

index 47acb36..5c546d0 100644
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+	struct clk *clk;
+	struct device_node *xen_node;
+	unsigned int i, count;
+	int ret = 0;
+
+	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+	if (!xen_node) {
+		pr_err("Xen support was detected before, but it has disappeared\n");
+		return -EINVAL;
+	}
+
+	count = of_clk_get_parent_count(xen_node);
+	if (!count)
+		goto out;
+
+	for (i = 0; i < count; i++) {
+		clk = of_clk_get(xen_node, i);
+		if (IS_ERR(clk)) {
+			pr_err("Xen failed to register clock %i. Error: %li\n",
+			       i, PTR_ERR(clk));
+			ret = PTR_ERR(clk);
+			goto out;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret < 0) {
+			pr_err("Xen failed to enable clock %i. Error: %i\n",
+			       i, ret);
+			goto out;
+		}
+	}
+
+	ret = 0;
+
+out:
+	of_node_put(xen_node);
+	return ret;
+}
+late_initcall(xen_arm_register_clks);
 
 /* empty stubs */
 void xen_arch_pre_suspend(void) { }

Signature · 1 line

2.8.0

Julien Grall

2016-07-05 13:54 UTC #9 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 10 lines · Stefano Stabellini

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 29

Quoted 8 lines · Dirk Behme

+- clocks: one or more clocks to be registered.

Documentation/devicetree/bindings/arm/xen.txt +7 -0

+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.

Quoted 2 lines

What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

clocks could be shared between multiple devices. So it is not possible to disable the clock.

Signature · 1 line

Julien Grall

Julien Grall

2016-07-05 14:02 UTC #10 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 16 lines · Julien Grall

Quoted 13 lines · Stefano Stabellini
Quoted 11 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +5 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the
clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.

Quoted 5 lines

Quoted 2 lines
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
clocks could be shared between multiple devices. So it is not possible to disable the clock.

To give an example, below the UART node for juno
(arch/arm64/boot/dts/arm/juno-base.dtsi):

         soc_uart0: uart at 7ff80000 {
                 compatible = "arm,pl011", "arm,primecell";
                 reg = <0x0 0x7ff80000 0x0 0x1000>;
                 interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
                 clocks = <&soc_uartclk>, <&soc_refclk100mhz>;
                 clock-names = "uartclk", "apb_pclk";
         };

The clock soc_refclk100mhz is shared with the mailbox.

Regards,

Signature · 1 line

Julien Grall

Julien Grall

2016-07-05 14:02 UTC #11 lore raw

Quoted 16 lines · Julien Grall

Quoted 13 lines · Stefano Stabellini
Quoted 11 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +5 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the
clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.

Quoted 5 lines

Quoted 2 lines
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
clocks could be shared between multiple devices. So it is not possible to disable the clock.

To give an example, below the UART node for juno (arch/arm64/boot/dts/arm/juno-base.dtsi):

        soc_uart0: uart@7ff80000 {
                compatible = "arm,pl011", "arm,primecell";
                reg = <0x0 0x7ff80000 0x0 0x1000>;
                interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
                clocks = <&soc_uartclk>, <&soc_refclk100mhz>;
                clock-names = "uartclk", "apb_pclk";
        };

The clock soc_refclk100mhz is shared with the mailbox.

Regards,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Stefano Stabellini

maintainer

2016-07-05 14:04 UTC #12 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

[1]
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00335.html

Quoted 12 lines · Julien Grall

Quoted 11 lines · Stefano Stabellini

Quoted 9 lines · Dirk Behme

+- clocks: one or more clocks to be registered.

Documentation/devicetree/bindings/arm/xen.txt +7 -0

+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is

called.

Quoted 5 lines

Quoted 2 lines
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
clocks could be shared between multiple devices. So it is not possible to disable the clock.

To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices.

If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV.

Julien Grall

2016-07-05 14:08 UTC #13 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 16 lines · Stefano Stabellini

Quoted 15 lines · Julien Grall
Quoted 13 lines · Stefano Stabellini
Quoted 9 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +7 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is
called.
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

Quoted 8 lines

Quoted 2 lines
clocks could be shared between multiple devices. So it is not possible to disable the clock.
To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices.

How would you be able to differentiate in Xen between a clock shared and a non-shared one? The only way I can think it going through all the device tree which sounds really expensive.

Quoted 4 lines

If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV.

We also want to prevent Linux changing the rate of the clock (see Mark's mail [1]).

Regards,

Signature · 1 line

Julien Grall

Stefano Stabellini

maintainer

2016-07-05 14:37 UTC #14 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 29 lines · Julien Grall

Quoted 19 lines · Stefano Stabellini
Quoted 18 lines · Julien Grall
Quoted 16 lines · Stefano Stabellini
Quoted 7 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +3 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by
clk_disable_unused(),
(quoted diff) +2 -0
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the
Quoted 5 lines
clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
Quoted 8 lines
Quoted 2 lines
clocks could be shared between multiple devices. So it is not possible to disable the clock.
To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices.

Quoted 3 lines

How would you be able to differentiate in Xen between a clock shared and a non-shared one? The only way I can think it going through all the device tree which sounds really expensive.

Yes, but it is only done once and we only need to do the search for clocks of "disabled" devices.

Quoted 7 lines

Quoted 4 lines
If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV.
We also want to prevent Linux changing the rate of the clock (see Mark's mail [1]).

I am not sure what's the best way to achieve that for clocks shared across multiple devices. Let's see what the clock maintainers suggest.

Stefano Stabellini

maintainer

2016-07-05 14:37 UTC #15 lore raw

Quoted 29 lines · Julien Grall

Quoted 19 lines · Stefano Stabellini
Quoted 18 lines · Julien Grall
Quoted 16 lines · Stefano Stabellini
Quoted 7 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +3 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by
clk_disable_unused(),
(quoted diff) +2 -0
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the
Quoted 5 lines
clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
Quoted 8 lines
Quoted 2 lines
clocks could be shared between multiple devices. So it is not possible to disable the clock.
To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices.

Quoted 3 lines

How would you be able to differentiate in Xen between a clock shared and a non-shared one? The only way I can think it going through all the device tree which sounds really expensive.

Yes, but it is only done once and we only need to do the search for clocks of "disabled" devices.

Quoted 7 lines

Quoted 4 lines
If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV.
We also want to prevent Linux changing the rate of the clock (see Mark's mail [1]).

I am not sure what's the best way to achieve that for clocks shared across multiple devices. Let's see what the clock maintainers suggest.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2016-07-05 14:08 UTC #16 lore raw

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00335.html

Quoted 16 lines · Stefano Stabellini

Quoted 15 lines · Julien Grall
Quoted 13 lines · Stefano Stabellini
Quoted 9 lines · Dirk Behme
+- clocks: one or more clocks to be registered.
Documentation/devicetree/bindings/arm/xen.txt +7 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is
called.
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

Quoted 8 lines

Quoted 2 lines
clocks could be shared between multiple devices. So it is not possible to disable the clock.
To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices.

How would you be able to differentiate in Xen between a clock shared and a non-shared one? The only way I can think it going through all the device tree which sounds really expensive.

Quoted 4 lines

If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV.

We also want to prevent Linux changing the rate of the clock (see Mark's mail [1]).

Regards,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Stefano Stabellini

maintainer

2016-07-05 14:04 UTC #17 lore raw

Quoted 12 lines · Julien Grall

Quoted 11 lines · Stefano Stabellini

Quoted 9 lines · Dirk Behme

+- clocks: one or more clocks to be registered.

Documentation/devicetree/bindings/arm/xen.txt +7 -0

+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is

called.

Quoted 5 lines

Quoted 2 lines
What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?
clocks could be shared between multiple devices. So it is not possible to disable the clock.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2016-07-05 13:54 UTC #18 lore raw

Quoted 10 lines · Stefano Stabellini

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 29

Quoted 8 lines · Dirk Behme

+- clocks: one or more clocks to be registered.

Documentation/devicetree/bindings/arm/xen.txt +7 -0

+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.

Quoted 2 lines

What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

clocks could be shared between multiple devices. So it is not possible to disable the clock.

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Stefano Stabellini

maintainer

2016-07-05 13:53 UTC #19 lore raw

Quoted 41 lines · Dirk Behme

Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644
@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 29

Quoted 10 lines

Documentation/devicetree/bindings/arm/xen.txt +10 -0

+Optional properties:
+
+- clocks: one or more clocks to be registered.
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.

What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone?

Quoted 71 lines

Documentation/devicetree/bindings/arm/xen.txt +0 -0

 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:

arch/arm/xen/enlighten.c +47 -0

index 47acb36..5c546d0 100644
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+	struct clk *clk;
+	struct device_node *xen_node;
+	unsigned int i, count;
+	int ret = 0;
+
+	xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+	if (!xen_node) {
+		pr_err("Xen support was detected before, but it has disappeared\n");
+		return -EINVAL;
+	}
+
+	count = of_clk_get_parent_count(xen_node);
+	if (!count)
+		goto out;
+
+	for (i = 0; i < count; i++) {
+		clk = of_clk_get(xen_node, i);
+		if (IS_ERR(clk)) {
+			pr_err("Xen failed to register clock %i. Error: %li\n",
+			       i, PTR_ERR(clk));
+			ret = PTR_ERR(clk);
+			goto out;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret < 0) {
+			pr_err("Xen failed to enable clock %i. Error: %i\n",
+			       i, ret);
+			goto out;
+		}
+	}
+
+	ret = 0;
+
+out:
+	of_node_put(xen_node);
+	return ret;
+}
+late_initcall(xen_arm_register_clks);
 
 /* empty stubs */
 void xen_arch_pre_suspend(void) { }

Signature · 1 line

2.8.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Michael Turquette

2016-07-06 01:34 UTC #20 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Hi!

Quoting Dirk Behme (2016-06-30 03:32:32)

Quoted 6 lines

Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.

This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).

This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.

Quoted 5 lines

Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45

clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

Quoted 27 lines

too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644

Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 22

Quoted 6 lines

Documentation/devicetree/bindings/arm/xen.txt +3 -0

@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 
+Optional properties:
+
+- clocks: one or more clocks to be registered.

s/registered/consumed/

For appropriate DT binding script to steal I picked one at random:

Documentation/devicetree/bindings/clock/ti/clockdomain.txt

Quoted 16 lines

Documentation/devicetree/bindings/arm/xen.txt +8 -0

+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:

arch/arm/xen/enlighten.c +0 -0

index 47acb36..5c546d0 100644

arch/arm/xen/enlighten.c line 27

Quoted 4 lines

arch/arm/xen/enlighten.c +1 -0

@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>

s/clk-provider.h/clk.h/

clk-provider.h is only used for providers and this bit of code is a consumer.

Quoted 21 lines

arch/arm/xen/enlighten.c +14 -0

 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+       struct clk *clk;
+       struct device_node *xen_node;
+       unsigned int i, count;
+       int ret = 0;
+
+       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+       if (!xen_node) {

arch/arm/xen/enlighten.c static int __init xen_pm_init(void) · line 471

Quoted 10 lines

arch/arm/xen/enlighten.c +10 -0

+               pr_err("Xen support was detected before, but it has disappeared\n");
+               return -EINVAL;
+       }
+
+       count = of_clk_get_parent_count(xen_node);
+       if (!count)
+               goto out;
+
+       for (i = 0; i < count; i++) {
+               clk = of_clk_get(xen_node, i);

Is there a struct device we can use here? It would be better to use devm_clk_get if possible.

Regards,
Mike

Quoted 28 lines

arch/arm/xen/enlighten.c +22 -0

+               if (IS_ERR(clk)) {
+                       pr_err("Xen failed to register clock %i. Error: %li\n",
+                              i, PTR_ERR(clk));
+                       ret = PTR_ERR(clk);
+                       goto out;
+               }
+
+               ret = clk_prepare_enable(clk);
+               if (ret < 0) {
+                       pr_err("Xen failed to enable clock %i. Error: %i\n",
+                              i, ret);
+                       goto out;
+               }
+       }
+
+       ret = 0;
+
+out:
+       of_node_put(xen_node);
+       return ret;
+}
+late_initcall(xen_arm_register_clks);
 
 /* empty stubs */
 void xen_arch_pre_suspend(void) { }

Signature · 1 line

2.8.0

Julien Grall

2016-07-06 13:10 UTC #21 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 1 line · Michael Turquette

Hi!

Hello Michael,

Quoted 26 lines

Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 4 lines

Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Regards,

Signature · 1 line

Julien Grall

Stefano Stabellini

maintainer

2016-07-06 13:16 UTC #22 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 31 lines · Julien Grall

Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 8 lines

Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct?

Julien Grall

2016-07-06 13:26 UTC #23 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 41 lines · Stefano Stabellini

Quoted 31 lines · Julien Grall
Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 8 lines
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Quoted 5 lines

Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct?

The problem is not "whether Linux would be happy or not", but the meaning of this property.

Based on the ePAPR, "status = disabled" indicates that the device is not presently operational, but it might become operational later (for example, something is not plugged in, or switched off).

An operating system which read this property should interpret as "the clock should not be used". However, with your suggestion the OS would need to differentiate between "the clock may be used by the hypervisor" and "the clock is not wired up/present/...".

Regards,

Signature · 1 line

Julien Grall

Julien Grall

2016-07-06 13:26 UTC #24 lore raw

Quoted 41 lines · Stefano Stabellini

Quoted 31 lines · Julien Grall
Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 8 lines
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Quoted 5 lines

Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct?

The problem is not "whether Linux would be happy or not", but the meaning of this property.

Based on the ePAPR, "status = disabled" indicates that the device is not presently operational, but it might become operational later (for example, something is not plugged in, or switched off).

Regards,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Mark Rutland

2016-07-06 13:48 UTC #25 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 37 lines · Stefano Stabellini

Quoted 36 lines · Julien Grall
Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 30 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

Quoted 8 lines

Quoted 3 lines
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices.

If you were to do that, that would cover the entire clock-controller, not necessarily for the individual clock line (as this does not necessarily have a node of its own). So you'd prevent the use of other clocks owned by that controller.

That's also not sufficient, as you'd have to do the same for resources required to keep that clock active (parent clocks from different controllers, regulators, GPIOs, etc).

I don't think that will work other than in very basic cases.

Thanks,
Mark.

Mark Rutland

2016-07-06 13:49 UTC #26 lore raw

Quoted 37 lines · Stefano Stabellini

Quoted 36 lines · Julien Grall
Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 30 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

Quoted 8 lines

Quoted 3 lines
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices.

That's also not sufficient, as you'd have to do the same for resources required to keep that clock active (parent clocks from different controllers, regulators, GPIOs, etc).

I don't think that will work other than in very basic cases.

Thanks,
Mark.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Stefano Stabellini

maintainer

2016-07-06 13:16 UTC #27 lore raw

Quoted 31 lines · Julien Grall

Quoted 1 line · Michael Turquette
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 8 lines

Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Michael Turquette

2016-07-06 20:42 UTC #28 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751

Hi Julien,

Quoting Julien Grall (2016-07-06 06:10:52)

Quoted 31 lines · Michael Turquette

Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 8 lines

Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.

Regards,
Mike

Quoted 5 lines

Regards,
Signature · 1 line
Julien Grall

Dirk Behme

author

2016-07-07 07:32 UTC #29 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Hi Michael,

Quoted 39 lines · Michael Turquette

Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 36 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 30 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

Quoted 6 lines

Quoted 3 lines
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.

Maybe we need some advice here :)

I've used clk_prepare_enable() 'just' to get the enable count incremented

Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.

If there is an other / better / correct way to achieve that, please let us know.

I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

Best regards

Dirk

Michael Turquette

2016-07-08 02:50 UTC #30 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoting Dirk Behme (2016-07-07 00:32:34)

Quoted 46 lines

Hi Michael,
Quoted 43 lines · Michael Turquette
Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 39 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Quoted 5 lines

Quoted 2 lines
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.
Maybe we need some advice here :)

Sure!

Quoted 3 lines

I've used clk_prepare_enable() 'just' to get the enable count incremented

clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns.

Quoted 5 lines

http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751
Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.

Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm).

The Sneaky Sibling Attack(tm) occurs when clock A and clock B are siblings and share the same parent, clock C. If clock A is enabled in hardware (by bootloader, firmware or hypervisor), but does NOT have a positive enable_count (in Linux), then it is possible that a driver might call clk_enable(clk_B) then clk_disable(clk_B), which will result in the disable action propagating up the parent chain and disabling clk_C, the shared parent. This will of course gate clk_A, which is clocked by clk_C, breaking things for you.

So you need to be worried about more than just clk_disable_unused.

The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable.

Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy.

Quoted 4 lines

If there is an other / better / correct way to achieve that, please let us know.

Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use".

Quoted 4 lines

I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

Right, the flags are immutable and must be set by the clock provider driver before registering the clock. Toggling flags at run-time is a misuse of the flags, and clock consumer drivers should never care about the flags. They are internal to the clock framework.

In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them. The two issues to resolve are:

1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model?
2) the language of the binding description makes this way more confusing than it needs to be. Just claim the resources you need and enable them, which is an OS-agnostic action.

Regards,
Mike

Quoted 5 lines

Best regards
Dirk

Dirk Behme

author

2016-07-08 05:51 UTC #31 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 105 lines · Michael Turquette

Quoting Dirk Behme (2016-07-07 00:32:34)
Quoted 46 lines
Hi Michael,
Quoted 43 lines · Michael Turquette
Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 39 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
Quoted 5 lines
Quoted 2 lines
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.
Maybe we need some advice here :)
Sure!
Quoted 3 lines
I've used clk_prepare_enable() 'just' to get the enable count incremented
clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns.
Quoted 5 lines
http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751
Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.
Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm).
The Sneaky Sibling Attack(tm) occurs when clock A and clock B are siblings and share the same parent, clock C. If clock A is enabled in hardware (by bootloader, firmware or hypervisor), but does NOT have a positive enable_count (in Linux), then it is possible that a driver might call clk_enable(clk_B) then clk_disable(clk_B), which will result in the disable action propagating up the parent chain and disabling clk_C, the shared parent. This will of course gate clk_A, which is clocked by clk_C, breaking things for you.
So you need to be worried about more than just clk_disable_unused.
The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable.
Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy.
Quoted 4 lines
If there is an other / better / correct way to achieve that, please let us know.
Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use".
Quoted 4 lines
I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

Quoted 8 lines

Right, the flags are immutable and must be set by the clock provider driver before registering the clock. Toggling flags at run-time is a misuse of the flags, and clock consumer drivers should never care about the flags. They are internal to the clock framework.
In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them.

Ok, thanks!

Quoted 4 lines

The two issues to resolve are:
1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model?

I think so. Julien, what do you think?

Quoted 3 lines

2) the language of the binding description makes this way more confusing than it needs to be. Just claim the resources you need and enable them, which is an OS-agnostic action.

I'll post a v3 patch trying to update the description with your proposals.

Best regards

Dirk

Julien Grall

2016-07-08 09:21 UTC #32 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Hi Dirk,

Quoted 9 lines · Dirk Behme

Quoted 5 lines · Michael Turquette
Quoting Dirk Behme (2016-07-07 00:32:34)
The two issues to resolve are:
1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model?
I think so. Julien, what do you think?

I don't think it fits the Xen model. I will give more details on the v3 as your commit message give an example with is contradictory to the model.

Regards,

Signature · 1 line

Julien Grall

Julien Grall

2016-07-08 09:21 UTC #33 lore raw

Hi Dirk,

Quoted 9 lines · Dirk Behme

Quoted 5 lines · Michael Turquette
Quoting Dirk Behme (2016-07-07 00:32:34)
The two issues to resolve are:
1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model?
I think so. Julien, what do you think?

I don't think it fits the Xen model. I will give more details on the v3 as your commit message give an example with is contradictory to the model.

Regards,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Dirk Behme

author

2016-07-08 05:51 UTC #34 lore raw

Quoted 105 lines · Michael Turquette

Quoting Dirk Behme (2016-07-07 00:32:34)
Quoted 46 lines
Hi Michael,
Quoted 43 lines · Michael Turquette
Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 39 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
Quoted 5 lines
Quoted 2 lines
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.
Maybe we need some advice here :)
Sure!
Quoted 3 lines
I've used clk_prepare_enable() 'just' to get the enable count incremented
clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns.
Quoted 5 lines
http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751
Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.
Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm).
The Sneaky Sibling Attack(tm) occurs when clock A and clock B are siblings and share the same parent, clock C. If clock A is enabled in hardware (by bootloader, firmware or hypervisor), but does NOT have a positive enable_count (in Linux), then it is possible that a driver might call clk_enable(clk_B) then clk_disable(clk_B), which will result in the disable action propagating up the parent chain and disabling clk_C, the shared parent. This will of course gate clk_A, which is clocked by clk_C, breaking things for you.
So you need to be worried about more than just clk_disable_unused.
The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable.
Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy.
Quoted 4 lines
If there is an other / better / correct way to achieve that, please let us know.
Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use".
Quoted 4 lines
I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

Quoted 8 lines

Right, the flags are immutable and must be set by the clock provider driver before registering the clock. Toggling flags at run-time is a misuse of the flags, and clock consumer drivers should never care about the flags. They are internal to the clock framework.
In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them.

Ok, thanks!

Quoted 4 lines

The two issues to resolve are:
1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model?

I think so. Julien, what do you think?

Quoted 3 lines

2) the language of the binding description makes this way more confusing than it needs to be. Just claim the resources you need and enable them, which is an OS-agnostic action.

I'll post a v3 patch trying to update the description with your proposals.

Best regards

Dirk

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Michael Turquette

2016-07-08 02:51 UTC #35 lore raw

Quoting Dirk Behme (2016-07-07 00:32:34)

Quoted 46 lines

Hi Michael,
Quoted 43 lines · Michael Turquette
Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 39 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Quoted 5 lines

Quoted 2 lines
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.
Maybe we need some advice here :)

Sure!

Quoted 3 lines

I've used clk_prepare_enable() 'just' to get the enable count incremented

clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns.

Quoted 5 lines

http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751
Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.

Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm).

So you need to be worried about more than just clk_disable_unused.

The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable.

Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy.

Quoted 4 lines

If there is an other / better / correct way to achieve that, please let us know.

Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use".

Quoted 4 lines

I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them. The two issues to resolve are:

Regards,
Mike

Quoted 5 lines

Best regards
Dirk

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Dirk Behme

author

2016-07-07 07:32 UTC #36 lore raw

http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751

Hi Michael,

Quoted 39 lines · Michael Turquette

Hi Julien,
Quoting Julien Grall (2016-07-06 06:10:52)
Quoted 36 lines · Michael Turquette
Quoted 1 line
Hi!
Hello Michael,
Quoted 30 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

Quoted 6 lines

Quoted 3 lines
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".
But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.

Maybe we need some advice here :)

I've used clk_prepare_enable() 'just' to get the enable count incremented

Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock.

If there is an other / better / correct way to achieve that, please let us know.

I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?)

Best regards

Dirk

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Michael Turquette

2016-07-06 20:44 UTC #37 lore raw

Hi Julien,

Quoting Julien Grall (2016-07-06 06:10:52)

Quoted 31 lines · Michael Turquette

Quoted 1 line
Hi!
Hello Michael,
Quoted 26 lines
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 8 lines

Quoted 4 lines
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused.

Regards,
Mike

Quoted 5 lines

Regards,
Signature · 1 line
Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Julien Grall

2016-07-06 13:10 UTC #38 lore raw

Quoted 1 line · Michael Turquette

Hi!

Hello Michael,

Quoted 26 lines

Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.

Quoted 4 lines

Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.

I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you".

Regards,

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Dirk Behme

author

2016-07-08 06:48 UTC #39 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Hi,

Quoted 96 lines · Michael Turquette

Hi!
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
Quoted 27 lines
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644
Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 22
Quoted 6 lines
Documentation/devicetree/bindings/arm/xen.txt +3 -0
@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 
+Optional properties:
+
+- clocks: one or more clocks to be registered.
s/registered/consumed/
For appropriate DT binding script to steal I picked one at random:
Documentation/devicetree/bindings/clock/ti/clockdomain.txt
Quoted 16 lines
Documentation/devicetree/bindings/arm/xen.txt +8 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:
 
arch/arm/xen/enlighten.c +0 -0
index 47acb36..5c546d0 100644
arch/arm/xen/enlighten.c line 27
Quoted 4 lines
arch/arm/xen/enlighten.c +1 -0
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>
s/clk-provider.h/clk.h/

Quoted 2 lines

clk-provider.h is only used for providers and this bit of code is a consumer.

It seems we need clk-provider.h for of_clk_get_parent_count()?

Quoted 25 lines

arch/arm/xen/enlighten.c +18 -0

 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+       struct clk *clk;
+       struct device_node *xen_node;
+       unsigned int i, count;
+       int ret = 0;
+
+       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+       if (!xen_node) {
+               pr_err("Xen support was detected before, but it has disappeared\n");
+               return -EINVAL;
+       }
+

Quoted 8 lines

arch/arm/xen/enlighten.c static int __init xen_pm_init(void) · line 471
Quoted 6 lines
arch/arm/xen/enlighten.c +6 -0
+       count = of_clk_get_parent_count(xen_node);
+       if (!count)
+               goto out;
+
+       for (i = 0; i < count; i++) {
+               clk = of_clk_get(xen_node, i);
Is there a struct device we can use here?

It doesn't look so. Julien?

Quoted 2 lines

It would be better to use
devm_clk_get if possible.

Best regards

Dirk

Julien Grall

2016-07-08 09:35 UTC #40 lore raw

PATCH v2 1/1 xen/arm: register clocks used by the hypervisor

Quoted 36 lines · Dirk Behme

Quoted 26 lines

#include <linux/cpuidle.h>
#include <linux/cpufreq.h>
#include <linux/cpu.h>

arch/arm/xen/enlighten.c +18 -0

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+       struct clk *clk;
+       struct device_node *xen_node;
+       unsigned int i, count;
+       int ret = 0;
+
+       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+       if (!xen_node) {
+               pr_err("Xen support was detected before, but it has
 disappeared\n");
+               return -EINVAL;
+       }
+

Quoted 8 lines

arch/arm/xen/enlighten.c static int __init xen_pm_init(void) · line 471
Quoted 6 lines
arch/arm/xen/enlighten.c +6 -0
+       count = of_clk_get_parent_count(xen_node);
+       if (!count)
+               goto out;
+
+       for (i = 0; i < count; i++) {
+               clk = of_clk_get(xen_node, i);
Is there a struct device we can use here?

Quoted 1 line

It doesn't look so. Julien?

We don't have a struct device. Maybe we can create a dummy one if it simplifies the logic?

Signature · 1 line

Julien Grall

Julien Grall

2016-07-08 09:36 UTC #41 lore raw

Quoted 36 lines · Dirk Behme

Quoted 26 lines

#include <linux/cpuidle.h>
#include <linux/cpufreq.h>
#include <linux/cpu.h>

arch/arm/xen/enlighten.c +18 -0

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+       struct clk *clk;
+       struct device_node *xen_node;
+       unsigned int i, count;
+       int ret = 0;
+
+       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+       if (!xen_node) {
+               pr_err("Xen support was detected before, but it has
 disappeared\n");
+               return -EINVAL;
+       }
+

Quoted 8 lines

arch/arm/xen/enlighten.c static int __init xen_pm_init(void) · line 471
Quoted 6 lines
arch/arm/xen/enlighten.c +6 -0
+       count = of_clk_get_parent_count(xen_node);
+       if (!count)
+               goto out;
+
+       for (i = 0; i < count; i++) {
+               clk = of_clk_get(xen_node, i);
Is there a struct device we can use here?

Quoted 1 line

It doesn't look so. Julien?

We don't have a struct device. Maybe we can create a dummy one if it simplifies the logic?

Signature · 6 lines

Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Dirk Behme

author

2016-07-08 06:48 UTC #42 lore raw

Hi,

Quoted 96 lines · Michael Turquette

Hi!
Quoting Dirk Behme (2016-06-30 03:32:32)
Quoted 6 lines
Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below.
Quoted 5 lines
Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them.
Quoted 27 lines
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled.
Signed-off-by Dirk Behme <[email protected]>
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
 Documentation/devicetree/bindings/arm/xen.txt | 11 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
Documentation/devicetree/bindings/arm/xen.txt +0 -0
index c9b9321..55dfd3b 100644
Documentation/devicetree/bindings/arm/xen.txt the following properties: · line 22
Quoted 6 lines
Documentation/devicetree/bindings/arm/xen.txt +3 -0
@@ -17,6 +17,17 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 
+Optional properties:
+
+- clocks: one or more clocks to be registered.
s/registered/consumed/
For appropriate DT binding script to steal I picked one at random:
Documentation/devicetree/bindings/clock/ti/clockdomain.txt
Quoted 16 lines
Documentation/devicetree/bindings/arm/xen.txt +8 -0
+  Xen hypervisor drivers might replace native drivers, resulting in
+  clocks not registered by these native drivers. To avoid that these
+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+  register them in the hypervisor node.
+  An example for this are the clocks of the serial driver. If the clocks
+  used by the serial hardware interface are not registered by the serial
+  driver the serial output might stop once clk_disable_unused() is called.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:
 
arch/arm/xen/enlighten.c +0 -0
index 47acb36..5c546d0 100644
arch/arm/xen/enlighten.c line 27
Quoted 4 lines
arch/arm/xen/enlighten.c +1 -0
@@ -24,6 +24,7 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/clk-provider.h>
s/clk-provider.h/clk.h/

Quoted 2 lines

clk-provider.h is only used for providers and this bit of code is a consumer.

It seems we need clk-provider.h for of_clk_get_parent_count()?

Quoted 25 lines

arch/arm/xen/enlighten.c +18 -0

 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>

@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
 }
 late_initcall(xen_pm_init);
 
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+       struct clk *clk;
+       struct device_node *xen_node;
+       unsigned int i, count;
+       int ret = 0;
+
+       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+       if (!xen_node) {
+               pr_err("Xen support was detected before, but it has disappeared\n");
+               return -EINVAL;
+       }
+

Quoted 8 lines

arch/arm/xen/enlighten.c static int __init xen_pm_init(void) · line 471
Quoted 6 lines
arch/arm/xen/enlighten.c +6 -0
+       count = of_clk_get_parent_count(xen_node);
+       if (!count)
+               goto out;
+
+       for (i = 0; i < count; i++) {
+               clk = of_clk_get(xen_node, i);
Is there a struct device we can use here?

It doesn't look so. Julien?

Quoted 2 lines

It would be better to use
devm_clk_get if possible.

Best regards

Dirk

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Michael Turquette

2016-07-06 01:34 UTC #43 lore raw