[PATCH v4] xen/arm: Add a clock property

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
               https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like clk_disable_unused
	       in xen.txt.

 Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.
 
+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:
 

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }

-- 
2.8.0

Quoting Dirk Behme (2016-07-12 00:46:45)

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

	CLK_SET_RATE_GATE
	CLK_SET_PARENT_GATE

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

On 13.07.2016 00:26, Michael Turquette wrote:

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

	CLK_SET_RATE_GATE
	CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think 
the conclusion was that in our case its not possible to manipulate the 
flags in the OS as this isn't intended to be done in cases like ours. 
Therefore no API is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we 
add the clocks to the hypervisor node in the device tree. And not in the 
kernel patch discussed here.

Best regards

Dirk

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
               https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like clk_disable_unused
               in xen.txt.

 Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
 arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
 under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }
--

On Tue, 12 Jul 2016, Dirk Behme wrote:

Given that this is a late initcall, the following should work:

    if (!xen_domain())
        return -ENODEV;

On Wed, 13 Jul 2016, Dirk Behme wrote:

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

	CLK_SET_RATE_GATE
	CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think the
conclusion was that in our case its not possible to manipulate the flags in
the OS as this isn't intended to be done in cases like ours. Therefore no API
is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we add the
clocks to the hypervisor node in the device tree. And not in the kernel patch
discussed here.

These are internal Linux flags, aren't they? They cannot be set in Xen.

If the only way to make sure that clk_set_rate and clk_set_parent fail
on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
a new internal Linux API.

quoted

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
               https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
               in xen.txt.

 Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
 arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
   A GIC node is also required.
   This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
 To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
 under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }
--

On 13.07.2016 20:35, Stefano Stabellini wrote:

On Tue, 12 Jul 2016, Dirk Behme wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like clk_disable_unused
	       in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47 +++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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;
+	}

Given that this is a late initcall, the following should work:

     if (!xen_domain())
         return -ENODEV;

Hmm, sorry, "should work" for what?

We need the xen_node from device tree, anyway.

Best regards

Dirk

On 13.07.2016 20:43, Stefano Stabellini wrote:

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

	CLK_SET_RATE_GATE
	CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think the
conclusion was that in our case its not possible to manipulate the flags in
the OS as this isn't intended to be done in cases like ours. Therefore no API
is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we add the
clocks to the hypervisor node in the device tree. And not in the kernel patch
discussed here.

These are internal Linux flags, aren't they?

I've been under the impression that you can set clock "flags" via the 
device tree. Seems I need to re-check that ;)

Best regards

Dirk

They cannot be set in Xen.

If the only way to make sure that clk_set_rate and clk_set_parent fail
on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
a new internal Linux API.

quoted

quoted

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
                in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }
--

_______________________________________________
Xen-devel mailing list
Xen-devel at lists.xen.org
https://lists.xen.org/xen-devel

On Wed, 13 Jul 2016, Dirk Behme wrote:

On 13.07.2016 20:35, Stefano Stabellini wrote:

quoted

On Tue, 12 Jul 2016, Dirk Behme wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
	       in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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;
+	}

Given that this is a late initcall, the following should work:

     if (!xen_domain())
         return -ENODEV;

Hmm, sorry, "should work" for what?

As a Xen check, if (!xen_domain()) is the common pattern.

We need the xen_node from device tree, anyway.

In that case, what don't you just use the global xen_node in this file?

Quoting Dirk Behme (2016-07-13 11:56:30)

On 13.07.2016 20:43, Stefano Stabellini wrote:

quoted

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

    CLK_SET_RATE_GATE
    CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think the
conclusion was that in our case its not possible to manipulate the flags in
the OS as this isn't intended to be done in cases like ours. Therefore no API
is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we add the
clocks to the hypervisor node in the device tree. And not in the kernel patch
discussed here.

These are internal Linux flags, aren't they?

I've been under the impression that you can set clock "flags" via the 
device tree. Seems I need to re-check that ;)

Right, you cannot set flags from the device tree. Also, setting these
flags is done by the clock provider driver, not a consumer. Xen is the
consumer.

Regards,
Mike

Best regards

Dirk

quoted

They cannot be set in Xen.

If the only way to make sure that clk_set_rate and clk_set_parent fail
on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
a new internal Linux API.

quoted

quoted

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
                in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }
--

_______________________________________________
Xen-devel mailing list
Xen-devel at lists.xen.org
https://lists.xen.org/xen-devel

On 13.07.2016 21:07, Stefano Stabellini wrote:

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 20:35, Stefano Stabellini wrote:

quoted

On Tue, 12 Jul 2016, Dirk Behme wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
	       in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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;
+	}

Given that this is a late initcall, the following should work:

     if (!xen_domain())
         return -ENODEV;

Hmm, sorry, "should work" for what?

As a Xen check, if (!xen_domain()) is the common pattern.

quoted

We need the xen_node from device tree, anyway.

In that case, what don't you just use the global xen_node in this file?

With the recent code there is no global xen_node any more:

https://lists.xen.org/archives/html/xen-devel/2016-06/msg02878.html

-- cut --
The code in this file has changed quite a lot with the support of ACPI. 
For instance "xen_node" is not anymore a global variable. Can you rebase 
your rework on top of the branch for-linus-4.8?


You can find the branch in:

git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
-- cut --

Best regards

Dirk

On 13.07.2016 23:03, Michael Turquette wrote:

Quoting Dirk Behme (2016-07-13 11:56:30)

quoted

On 13.07.2016 20:43, Stefano Stabellini wrote:

quoted

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

    CLK_SET_RATE_GATE
    CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think the
conclusion was that in our case its not possible to manipulate the flags in
the OS as this isn't intended to be done in cases like ours. Therefore no API
is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we add the
clocks to the hypervisor node in the device tree. And not in the kernel patch
discussed here.

These are internal Linux flags, aren't they?

I've been under the impression that you can set clock "flags" via the
device tree. Seems I need to re-check that ;)

Right, you cannot set flags from the device tree. Also, setting these
flags is done by the clock provider driver, not a consumer. Xen is the
consumer.

Ok, thanks, then I think we can forget about using flags for the issue 
we are discussing here.

Best regards

Dirk

P.S.: Would it be an option to merge the v4 patch we are discussing 
here, then? From the discussion until here, it sounds to me that it's 
the best option we have at the moment. Maybe improving it in the future, 
then.

quoted

quoted

They cannot be set in Xen.

If the only way to make sure that clk_set_rate and clk_set_parent fail
on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
a new internal Linux API.

quoted

quoted

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
                in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }

On Wed, 13 Jul 2016, Michael Turquette wrote:

Quoting Dirk Behme (2016-07-13 11:56:30)

quoted

On 13.07.2016 20:43, Stefano Stabellini wrote:

quoted

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

    CLK_SET_RATE_GATE
    CLK_SET_PARENT_GATE

Regarding setting flags, I think we already talked about that. I think the
conclusion was that in our case its not possible to manipulate the flags in
the OS as this isn't intended to be done in cases like ours. Therefore no API
is exported for this.

I.e. if we need to set these flags, we have to do that in Xen where we add the
clocks to the hypervisor node in the device tree. And not in the kernel patch
discussed here.

These are internal Linux flags, aren't they?

I've been under the impression that you can set clock "flags" via the 
device tree. Seems I need to re-check that ;)

Right, you cannot set flags from the device tree. Also, setting these
flags is done by the clock provider driver, not a consumer. Xen is the
consumer.

Just for disambiguation, I guess you are referring to the Xen code in
Linux (such as arch/arm/xen/enlighten.c), right?

We have "Xen", the hypervisor, which runs before Linux and it is a
separate component running at EL2, and we have Xen support in Linux,
such as arch/arm/xen and drivers/xen. These are drivers to communicate
with the Xen hypervisor and other components in a Xen system.

In this case, the Xen hypervisor would be the owner of the clock. Xen
support in Linux, specifically arch/arm/xen/enlighten.c, only comes into
the picture to "warn" Linux that shouldn't really be messing with the
clocks. I guess that from a Linux clock driver point of view, it is a
consumer.

I hope that helps.

quoted

quoted

They cannot be set in Xen.

If the only way to make sure that clk_set_rate and clk_set_parent fail
on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
a new internal Linux API.

quoted

quoted

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

quoted

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

quoted

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael. Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
                in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and the
+  OS must not alter their state any way, such as disabling or gating a
+  clock, or modifying its rate. Ensuring this may impose constraints on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT

"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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) { }
--

_______________________________________________
Xen-devel mailing list
Xen-devel at lists.xen.org
https://lists.xen.org/xen-devel

On Thu, 14 Jul 2016, Dirk Behme wrote:

On 13.07.2016 21:07, Stefano Stabellini wrote:

quoted

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 20:35, Stefano Stabellini wrote:

quoted

On Tue, 12 Jul 2016, Dirk Behme wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the
OS
must not alter their state any way, such as disabling or gating a
clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken
ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

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.

Signed-off-by: Dirk Behme <redacted>
---
Changes in v4: Switch to the xen.txt description proposed by Mark:
                https://www.spinics.net/lists/arm-kernel/msg516158.html

Changes in v3: Use the xen.txt description proposed by Michael.
Thanks!

Changes in v2: Drop the Linux implementation details like
clk_disable_unused
	       in xen.txt.

  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
  arch/arm/xen/enlighten.c                      | 47
+++++++++++++++++++++++++++
  2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/xen.txt

b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..437e50b 100644

--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt

@@ -17,6 +17,18 @@ the following properties:
    A GIC node is also required.
    This property is unnecessary when booting Dom0 using ACPI.

+Optional properties:
+
+- clocks: a list of phandle + clock-specifier pairs
+  Clocks described by this property are reserved for use by Xen, and

the
+  OS must not alter their state any way, such as disabling or gating
a
+  clock, or modifying its rate. Ensuring this may impose constraints
on
+  parent clocks or other resources used by the clock tree.
+
+  Note: this property is used to proxy clocks for devices Xen has
taken
+  ownership of, such as UARTs, for which the associated clock
+  controller(s) remain under the control of Dom0.
+
  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT
"uefi" node
  under /hypervisor with following parameters:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c

@@ -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;
+	}

Given that this is a late initcall, the following should work:

     if (!xen_domain())
         return -ENODEV;

Hmm, sorry, "should work" for what?

As a Xen check, if (!xen_domain()) is the common pattern.

quoted

We need the xen_node from device tree, anyway.

In that case, what don't you just use the global xen_node in this file?

With the recent code there is no global xen_node any more:

https://lists.xen.org/archives/html/xen-devel/2016-06/msg02878.html

Ops, I was looking at upstream. I forgot about those.

Hi Dirk,

On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme [off-list ref] wrote:

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm not familiar with using XEN at all, but I'm a bit puzzled...

Can't you just add a clocks property to the (virtual) serial device node in DT?
Then the (virtual) serial device driver can get and enable the clock?

Alternatively, you can add a (virtual) clock controller, and power-domains
and clock properties to all affected devices (I assume there can be others,
besides virtual UARTs?), and let it be handled by Runtime PM, without the
(virtual) device drivers having to care about clocks at all.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On 20/07/16 10:43, Geert Uytterhoeven wrote:

Hi Dirk,

Hi Geert,

On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme [off-list ref] wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm not familiar with using XEN at all, but I'm a bit puzzled...

Can't you just add a clocks property to the (virtual) serial device node in DT?
Then the (virtual) serial device driver can get and enable the clock?

There is no DT node for the Xen console (hvc). The UART used by Xen will 
be completely removed from the Device tree.

Alternatively, you can add a (virtual) clock controller, and power-domains
and clock properties to all affected devices (I assume there can be others,
besides virtual UARTs?), and let it be handled by Runtime PM, without the
(virtual) device drivers having to care about clocks at all.

I am not sure to understand here. The problem is not because we provide 
virtual device to DOM0 but because the physical devices will be 
completely hidden (i.e remove from the DT) from DOM0.

Those devices will be removed from the Device Tree and may not have a 
corresponding virtual device. So we cannot replicate the clocks property 
in the device.

In a previous mail [1], Stefano suggested to add a new property for the 
clock to mention that the clock should not changed (i.e rate, 
disable...). How would that fit?

Regards,

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

-- 
Julien Grall

Hi Julien,

On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall [off-list ref] wrote:

On 20/07/16 10:43, Geert Uytterhoeven wrote:

quoted

On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme [off-list ref]
wrote:

quoted

Clocks described by this property are reserved for use by Xen, and the OS
must not alter their state any way, such as disabling or gating a clock,
or modifying its rate. Ensuring this may impose constraints on parent
clocks or other resources used by the clock tree.

This property is used to proxy clocks for devices Xen has taken ownership
of, such as UARTs, for which the associated clock controller(s) remain
under the control of Dom0.

I'm not familiar with using XEN at all, but I'm a bit puzzled...

Can't you just add a clocks property to the (virtual) serial device node
in DT?
Then the (virtual) serial device driver can get and enable the clock?

There is no DT node for the Xen console (hvc). The UART used by Xen will be
completely removed from the Device tree.

Why is it removed?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds