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

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

[Dirk Behme]

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 <dirk.behme@de.bosch.com>
---
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

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

[Michael Turquette]

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

> 
> 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 <dirk.behme@de.bosch.com>
> ---
> 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
> 

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

[Dirk Behme]

On 13.07.2016 00:26, Michael Turquette wrote:
> 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



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.
>
>>
>> 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
>
>>
>> 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 <dirk.behme@de.bosch.com>
>> ---
>> 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) { }
>> --

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

[Stefano Stabellini]

On Tue, 12 Jul 2016, Dirk Behme 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.
> 
> 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 <dirk.behme@de.bosch.com>
> ---
> 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;

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

[Stefano Stabellini]

On Wed, 13 Jul 2016, Dirk Behme wrote:
> On 13.07.2016 00:26, Michael Turquette wrote:
> > 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
> 
> 
> 
> 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.


> > 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
> > 
> > > 
> > > 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 <dirk.behme@de.bosch.com>
> > > ---
> > > 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] [PATCH v4] xen/arm: Add a clock property

[Dirk Behme]

On 13.07.2016 20:35, Stefano Stabellini wrote:
> On Tue, 12 Jul 2016, Dirk Behme 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.
>>
>> 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 <dirk.behme@de.bosch.com>
>> ---
>> 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

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

[Dirk Behme]

On 13.07.2016 20:43, Stefano Stabellini wrote:
> On Wed, 13 Jul 2016, Dirk Behme wrote:
>> On 13.07.2016 00:26, Michael Turquette wrote:
>>> 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
>>
>>
>>
>> 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.
>
>
>>> 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
>>>
>>>>
>>>> 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 <dirk.behme@de.bosch.com>
>>>> ---
>>>> 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
>

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

[Stefano Stabellini]

On Wed, 13 Jul 2016, Dirk Behme wrote:
> On 13.07.2016 20:35, Stefano Stabellini wrote:
> > On Tue, 12 Jul 2016, Dirk Behme 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.
> > > 
> > > 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 <dirk.behme@de.bosch.com>
> > > ---
> > > 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?

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

[Michael Turquette]

Quoting Dirk Behme (2016-07-13 11:56:30)
> On 13.07.2016 20:43, Stefano Stabellini wrote:
> > On Wed, 13 Jul 2016, Dirk Behme wrote:
> >> On 13.07.2016 00:26, Michael Turquette wrote:
> >>> 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
> >>
> >>
> >>
> >> 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
> 
> 
> > 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.
> >
> >
> >>> 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
> >>>
> >>>>
> >>>> 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 <dirk.behme@de.bosch.com>
> >>>> ---
> >>>> 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
> >
> 

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

[Dirk Behme]

On 13.07.2016 21:07, Stefano Stabellini wrote:
> On Wed, 13 Jul 2016, Dirk Behme wrote:
>> On 13.07.2016 20:35, Stefano Stabellini wrote:
>>> On Tue, 12 Jul 2016, Dirk Behme 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.
>>>>
>>>> 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 <dirk.behme@de.bosch.com>
>>>> ---
>>>> 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?


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

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

[Dirk Behme]

On 13.07.2016 23:03, Michael Turquette wrote:
> Quoting Dirk Behme (2016-07-13 11:56:30)
>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>> 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
>>>>
>>>>
>>>>
>>>> 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.


>>> 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.
>>>
>>>
>>>>> 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
>>>>>
>>>>>>
>>>>>> 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 <dirk.behme@de.bosch.com>
>>>>>> ---
>>>>>> 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] [PATCH v4] xen/arm: Add a clock property

[Julien Grall]

Hi Dirk,

On 14/07/16 07:31, Dirk Behme wrote:
> On 13.07.2016 23:03, Michael Turquette wrote:
>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>> 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.

I think it is a good start, although I would like to see some rationale 
in the code and commit message about the behavior of the Linux with 
those clocks after this patch because it does not match the "contract".

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 13 Jul 2016, Michael Turquette wrote:
> Quoting Dirk Behme (2016-07-13 11:56:30)
> > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > >> On 13.07.2016 00:26, Michael Turquette wrote:
> > >>> 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
> > >>
> > >>
> > >>
> > >> 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.


> > > 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.
> > >
> > >
> > >>> 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
> > >>>
> > >>>>
> > >>>> 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 <dirk.behme@de.bosch.com>
> > >>>> ---
> > >>>> 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
> > >
> > 
> 

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

[Stefano Stabellini]

On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 13.07.2016 21:07, Stefano Stabellini wrote:
> > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > On 13.07.2016 20:35, Stefano Stabellini wrote:
> > > > On Tue, 12 Jul 2016, Dirk Behme 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.
> > > > > 
> > > > > 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 <dirk.behme@de.bosch.com>
> > > > > ---
> > > > > 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?
> 
> 
> 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.

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

[Dirk Behme]

On 14.07.2016 12:14, Julien Grall wrote:
> Hi Dirk,
>
> On 14/07/16 07:31, Dirk Behme wrote:
>> On 13.07.2016 23:03, Michael Turquette wrote:
>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>
> I think it is a good start, although I would like to see some rationale
> in the code and commit message about the behavior of the Linux with
> those clocks after this patch because it does not match the "contract".


I'd be happy about any wording proposals :)


Best regards

Dirk

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

[Stefano Stabellini]

On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 13.07.2016 23:03, Michael Turquette wrote:
> > Quoting Dirk Behme (2016-07-13 11:56:30)
> > > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > > > On 13.07.2016 00:26, Michael Turquette wrote:
> > > > > > 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
> > > > > 
> > > > > 
> > > > > 
> > > > > 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.

It might be a step in the right direction, but it doesn't really prevent
clk_set_rate from changing properties of a clock owned by Xen.  This
patch is incomplete. We need to understand at least what it would take
to have a complete solution.

Michael, do you have any suggestions on how it would be possible to set
CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
way?

Like you wrote, I would imagine it needs to be done by the clock
provider driver. Maybe to do that, it would be easier to have a new
device tree property on the clock node, rather than listing phandle and
clock-specifier pairs under the Xen node?

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

[Dirk Behme]

On 14.07.2016 12:38, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Dirk Behme wrote:
>> On 13.07.2016 23:03, Michael Turquette wrote:
>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>
> It might be a step in the right direction, but it doesn't really prevent
> clk_set_rate from changing properties of a clock owned by Xen.  This
> patch is incomplete.


Let me ask then: Do we have a practical example where it's not 
sufficient practically?

To my understanding, Xen people have been happy with the 
"clk_ignore_unused" workaround since ~2 years, now [1]. And I think the 
"clk_ignore_unused" workaround does mainly the same like the patch 
discussed here. It doesn't care regarding clk_set_rate from changing 
properties, too?

While I agree that the patch theoretically incomplete, if nobody has a 
real world example I would think that from practical point of view it's 
sufficient in a first step.

If this is the case, I'd propose to fix the practical issue in a first 
step with a patch (this one) which is sufficient to fix the issues the 
Xen users have. And update the code for theoretical future issues in a 
second step.

Best regards

Dirk

[1] http://bugs.xenproject.org/xen/bug/45


> We need to understand at least what it would take
> to have a complete solution.
>
> Michael, do you have any suggestions on how it would be possible to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
> way?
>
> Like you wrote, I would imagine it needs to be done by the clock
> provider driver. Maybe to do that, it would be easier to have a new
> device tree property on the clock node, rather than listing phandle and
> clock-specifier pairs under the Xen node?

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

[Stefano Stabellini]

On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 14.07.2016 12:38, Stefano Stabellini wrote:
> > On Thu, 14 Jul 2016, Dirk Behme wrote:
> > > On 13.07.2016 23:03, Michael Turquette wrote:
> > > > Quoting Dirk Behme (2016-07-13 11:56:30)
> > > > > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > > > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > > > > > On 13.07.2016 00:26, Michael Turquette wrote:
> > > > > > > > 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
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 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.
> > 
> > It might be a step in the right direction, but it doesn't really prevent
> > clk_set_rate from changing properties of a clock owned by Xen.  This
> > patch is incomplete.
> 
> 
> Let me ask then: Do we have a practical example where it's not sufficient
> practically?
> 
> To my understanding, Xen people have been happy with the "clk_ignore_unused"
> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
> workaround does mainly the same like the patch discussed here. It doesn't care
> regarding clk_set_rate from changing properties, too?

Let me premise that I appreciate what you are trying to achieve with
this patch and I don't want to feature-creep it.

However we are defining a new Device Tree binding, one which will have
to be supported for a long time by both Xen and Linux, so at the very
least we need to have the full picture. We need to understand if the
binding if sufficient or if we need something different to solve the
problem completely.

Once we understand that, I am happy to accept a partial implementation
in Linux, as long as it is a step in the right direction. Does it make
sense?



> While I agree that the patch theoretically incomplete, if nobody has a real
> world example I would think that from practical point of view it's sufficient
> in a first step.
> 
> If this is the case, I'd propose to fix the practical issue in a first step
> with a patch (this one) which is sufficient to fix the issues the Xen users
> have. And update the code for theoretical future issues in a second step.
> 
> Best regards
> 
> Dirk
> 
> [1] http://bugs.xenproject.org/xen/bug/45
> 
> 
> > We need to understand at least what it would take
> > to have a complete solution.
> > 
> > Michael, do you have any suggestions on how it would be possible to set
> > CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
> > way?
> > 
> > Like you wrote, I would imagine it needs to be done by the clock
> > provider driver. Maybe to do that, it would be easier to have a new
> > device tree property on the clock node, rather than listing phandle and
> > clock-specifier pairs under the Xen node?
> 

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

[Dirk Behme]

On 14.07.2016 17:55, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Dirk Behme wrote:
>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>
>>> It might be a step in the right direction, but it doesn't really prevent
>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>> patch is incomplete.
>>
>>
>> Let me ask then: Do we have a practical example where it's not sufficient
>> practically?
>>
>> To my understanding, Xen people have been happy with the "clk_ignore_unused"
>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>> workaround does mainly the same like the patch discussed here. It doesn't care
>> regarding clk_set_rate from changing properties, too?
>
> Let me premise that I appreciate what you are trying to achieve with
> this patch and I don't want to feature-creep it.
>
> However we are defining a new Device Tree binding,


I don't think so.

We are just using the existing one

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66

, pick it from other device tree nodes (e.g. serial, timer etc) and 
add it to the hypervisor node. And then use this existing one with the 
existing well defined clock API.


> one which will have
> to be supported for a long time by both Xen and Linux, so at the very
> least we need to have the full picture. We need to understand if the
> binding if sufficient


Even if it's not sufficient, you can't change it.


> or if we need something different to solve the
> problem completely.


You might need anything additionally. E.g. an extension of the Linux 
kernel clock API to be able to modify the flags was proposed.

Best regards

Dirk

P.S.: I still would be interested if we do have a practical example 
where it's not sufficient practically?


> Once we understand that, I am happy to accept a partial implementation
> in Linux, as long as it is a step in the right direction. Does it make
> sense?
>
>
>
>> While I agree that the patch theoretically incomplete, if nobody has a real
>> world example I would think that from practical point of view it's sufficient
>> in a first step.
>>
>> If this is the case, I'd propose to fix the practical issue in a first step
>> with a patch (this one) which is sufficient to fix the issues the Xen users
>> have. And update the code for theoretical future issues in a second step.
>>
>> Best regards
>>
>> Dirk
>>
>> [1] http://bugs.xenproject.org/xen/bug/45
>>
>>
>>> We need to understand at least what it would take
>>> to have a complete solution.
>>>
>>> Michael, do you have any suggestions on how it would be possible to set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
>>> way?
>>>
>>> Like you wrote, I would imagine it needs to be done by the clock
>>> provider driver. Maybe to do that, it would be easier to have a new
>>> device tree property on the clock node, rather than listing phandle and
>>> clock-specifier pairs under the Xen node?
>>
>

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

[Julien Grall]

Hello,

On 14/07/16 17:30, Dirk Behme wrote:
> On 14.07.2016 17:55, Stefano Stabellini wrote:
>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>
>>>> It might be a step in the right direction, but it doesn't really
>>>> prevent
>>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>>> patch is incomplete.
>>>
>>>
>>> Let me ask then: Do we have a practical example where it's not
>>> sufficient
>>> practically?
>>>
>>> To my understanding, Xen people have been happy with the
>>> "clk_ignore_unused"
>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>> workaround does mainly the same like the patch discussed here. It
>>> doesn't care
>>> regarding clk_set_rate from changing properties, too?
>>
>> Let me premise that I appreciate what you are trying to achieve with
>> this patch and I don't want to feature-creep it.
>>
>> However we are defining a new Device Tree binding,
>
>
> I don't think so.
>
> We are just using the existing one
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>
>
> , pick it from other device tree nodes (e.g. serial, timer etc) and add
> it to the hypervisor node. And then use this existing one with the
> existing well defined clock API.
>
>
>> one which will have
>> to be supported for a long time by both Xen and Linux, so at the very
>> least we need to have the full picture. We need to understand if the
>> binding if sufficient
>
>
> Even if it's not sufficient, you can't change it.

I think you misunderstood Stefano's comment. Whilst the clock bindings 
is set in stone, the binding you are adding in this patch is not yet 
fixed. There is no requirement to follow what was defined in 
clock-bindings.txt. I agree it would be convenient, but as mentioned by 
Stefano this will need to be supported for a long time by Xen, Linux, 
and any other consumer (i.e BSD kernels). So we have to be careful on 
how it has been defined.

I would wait the answer of Michael on Stefano's question before taking 
any decision here.


>
>
>> or if we need something different to solve the
>> problem completely.
>
>
> You might need anything additionally. E.g. an extension of the Linux
> kernel clock API to be able to modify the flags was proposed.

The Linux kernel is not the only consumer of the device tree bindings. 
This is also used by other OS such as FreeBSD where you might already be 
able (I have not actually checked) to forbid a user to change the clock 
rate.

>
> Best regards
>
> Dirk
>
> P.S.: I still would be interested if we do have a practical example
> where it's not sufficient practically?

Very easy. What does prevent a driver to change the clock rate? Nothing 
but the flags mentioned by Michael. There are already drivers which 
modify the clock rate, thankfully those clocks are not shared with the 
UART for now.

But we cannot rule out that it will not be possible in the future. Think 
about a clock that would be used by another guest (I know it is still 
theoretical as we have not yet solved the problem).

Regards,

-- 
Julien Grall

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

[Dirk Behme]

On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>
>>>>> It might be a step in the right direction, but it doesn't really
>>>>> prevent
>>>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>>>> patch is incomplete.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.


It has to be

a) identical what we pick from UART, timer, etc

b) compatible to the kernel's clock API

No?

With these two requirements I have some difficulties to imagine how it 
could be different to the clock binding from clock-bindings.txt?



> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).


Sounds like an additional argument for clock-bindings.txt ;)


> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.


Fine with me :)


Best regards

Dirk


>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).

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

[Geert Uytterhoeven]

Hi Dirk,

On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> 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

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

[Julien Grall]

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

Hi Geert,

> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> 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?

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

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

[Geert Uytterhoeven]

Hi Julien,

On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote:
> On 20/07/16 10:43, Geert Uytterhoeven wrote:
>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
>> 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?
>
> 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

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

[Julien Grall]

On 20/07/16 12:49, Geert Uytterhoeven wrote:
> Hi Julien,
>
> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 20/07/16 10:43, Geert Uytterhoeven wrote:
>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> 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?
>>
>> 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?

Because the device is used exclusively by Xen and DOM0 should not touch 
it at all (IRQs and MMIOs are not mapped).

Regards,

-- 
Julien Grall

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

[Geert Uytterhoeven]

Hi Julien,

On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote:
> On 20/07/16 12:49, Geert Uytterhoeven wrote:
>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>> On 20/07/16 10:43, Geert Uytterhoeven wrote:
>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>>> 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?
>>>
>>> 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?
>
> Because the device is used exclusively by Xen and DOM0 should not touch it
> at all (IRQs and MMIOs are not mapped).

IMHO then it's Xen's responsability to make sure not to disable the clock(s).

Who removes the device node from the DT? If Xen, can't it just remember
which clocks were present in removed device nodes?

What to do on SoCs where the serial device is part of a power area (e.g.
Renesas SH/R-Mobile)? Who will make sure it is not powered down?

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

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

[Dirk Behme]

On 20.07.2016 14:46, Geert Uytterhoeven wrote:
> Hi Julien,
>
> On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 20/07/16 12:49, Geert Uytterhoeven wrote:
>>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>> On 20/07/16 10:43, Geert Uytterhoeven wrote:
>>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>>>> 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?
>>>>
>>>> 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?
>>
>> Because the device is used exclusively by Xen and DOM0 should not touch it
>> at all (IRQs and MMIOs are not mapped).
>
> IMHO then it's Xen's responsability to make sure not to disable the clock(s).
>
> Who removes the device node from the DT? If Xen, can't it just remember
> which clocks were present in removed device nodes?


Yes, we are trying this with "moving" the clocks of the removed nodes to 
the Xen hypervisor node:

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

The question discussed here is how to deal with these clocks properly in 
Linux, then.

Best regards

Dirk

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

[Julien Grall]

On 20/07/2016 13:46, Geert Uytterhoeven wrote:
> Hi Julien,

Hello Geert,

> On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 20/07/16 12:49, Geert Uytterhoeven wrote:
>>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>> On 20/07/16 10:43, Geert Uytterhoeven wrote:
>>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>>>> 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?
>>>>
>>>> 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?
>>
>> Because the device is used exclusively by Xen and DOM0 should not touch it
>> at all (IRQs and MMIOs are not mapped).
>
> IMHO then it's Xen's responsability to make sure not to disable the clock(s).

Xen does not disable the clock(s). The problem is Linux thinks the clock 
is not used and therefore will disable the clock.

Actually Xen does not have any knowledge how to handle clocks (i.e 
setting rate, enabling/disabling). I would recommend you to read the 
binding description in the patch and not the implementation which is 
IHMO misleading.

>
> Who removes the device node from the DT? If Xen, can't it just remember
> which clocks were present in removed device nodes?

Xen is removing the node from the DT. Not removing the node will not 
change the problem. Linux is disabling the clock because there is no 
driver which used those clocks.

As the clock subsystem will disable any unused clocks (from Linux point 
of view), the UART will get disabled and the console will be lost.

What we want to achieve with this patch is to let Linux knows that this 
clock is in use by someone else (hypervisor or guest) and:
     * If the clock is not shared: don't disable it
     * If the clock is shared: don't change the rate

>
> What to do on SoCs where the serial device is part of a power area (e.g.
> Renesas SH/R-Mobile)? Who will make sure it is not powered down?

I don't have any knowledge on the Renesas SH/R-Mobile. However, we 
expect some cooperation between DOM0 and Xen to handle the power.

For instance managing the clocks in Xen would require to implement clock 
driver for each SOC because it does not seem to have a generic way to do it.

Given that Linux already knows a lot about the clock, we want to hand 
over the clock control to the hardware domain (i.e dom0). This means 
that we have to find a way to cooperate with it to keep enable clocks 
that we care about.

Regards,

-- 
Julien Grall

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

[Michael Turquette]

Quoting Stefano Stabellini (2016-07-14 03:38:04)
> On Thu, 14 Jul 2016, Dirk Behme wrote:
> > On 13.07.2016 23:03, Michael Turquette wrote:
> > > Quoting Dirk Behme (2016-07-13 11:56:30)
> > > > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > > > > On 13.07.2016 00:26, Michael Turquette wrote:
> > > > > > > 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
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 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.
> 
> It might be a step in the right direction, but it doesn't really prevent
> clk_set_rate from changing properties of a clock owned by Xen.  This
> patch is incomplete. We need to understand at least what it would take
> to have a complete solution.
> 
> Michael, do you have any suggestions on how it would be possible to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
> way?

No, there is no way for a consumer to do that. The provider must do it.

> 
> Like you wrote, I would imagine it needs to be done by the clock
> provider driver. Maybe to do that, it would be easier to have a new
> device tree property on the clock node, rather than listing phandle and
> clock-specifier pairs under the Xen node?

Upon further reflection, I think that your clock consumer can probably
use clk_set_rate_range() to "lock" in a rate. This is good because it is
exactly what a clock consumer should do:

1) get the clk
2) enable the clk
3) set the required rate for the clock
4) set rate range constraints, or conversely,
5) lock in an exact rate; set the min/max rate to the same value

The problem with this solution is that it requires the consumer to have
knowledge of the rates that it wants for that clock, which I guess is
something that Linux kernels in a Xen setup do not want/need?

Is it correct that you would prefer some sort of never_touch_this_clk()
api?

Regards,
Mike

> 

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

[Michael Turquette]

Quoting Julien Grall (2016-07-20 06:21:45)
> 
> 
> On 20/07/2016 13:46, Geert Uytterhoeven wrote:
> > Hi Julien,
> 
> Hello Geert,
> 
> > On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote:
> >> On 20/07/16 12:49, Geert Uytterhoeven wrote:
> >>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com>
> >>> wrote:
> >>>> On 20/07/16 10:43, Geert Uytterhoeven wrote:
> >>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com>
> >>>>> 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?
> >>>>
> >>>> 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?
> >>
> >> Because the device is used exclusively by Xen and DOM0 should not touch it
> >> at all (IRQs and MMIOs are not mapped).
> >
> > IMHO then it's Xen's responsability to make sure not to disable the clock(s).
> 
> Xen does not disable the clock(s). The problem is Linux thinks the clock 
> is not used and therefore will disable the clock.
> 
> Actually Xen does not have any knowledge how to handle clocks (i.e 
> setting rate, enabling/disabling). I would recommend you to read the 
> binding description in the patch and not the implementation which is 
> IHMO misleading.
> 
> >
> > Who removes the device node from the DT? If Xen, can't it just remember
> > which clocks were present in removed device nodes?
> 
> Xen is removing the node from the DT. Not removing the node will not 
> change the problem. Linux is disabling the clock because there is no 
> driver which used those clocks.
> 
> As the clock subsystem will disable any unused clocks (from Linux point 
> of view), the UART will get disabled and the console will be lost.
> 
> What we want to achieve with this patch is to let Linux knows that this 
> clock is in use by someone else (hypervisor or guest) and:
>      * If the clock is not shared: don't disable it
>      * If the clock is shared: don't change the rate

Thanks for the explanation above. It helped clarify things for me.

So, can you have some sort of dummy driver in Linux that claims the
resources needed by Xen and calls the necessary Linux apis to insure
that Xen's needs are satisfied? As I mentioned in my previous reply
from a couple of minutes ago:

struct clk *clk = clk_get(xen_dev, ...);
clk_prepare_enable(clk);
clk_set_rate_range(...);

I'm not sure where the rate info should come from, but this represents a
correct use of clk consumer apis.

Regards,
Mike

> 
> >
> > What to do on SoCs where the serial device is part of a power area (e.g.
> > Renesas SH/R-Mobile)? Who will make sure it is not powered down?
> 
> I don't have any knowledge on the Renesas SH/R-Mobile. However, we 
> expect some cooperation between DOM0 and Xen to handle the power.
> 
> For instance managing the clocks in Xen would require to implement clock 
> driver for each SOC because it does not seem to have a generic way to do it.
> 
> Given that Linux already knows a lot about the clock, we want to hand 
> over the clock control to the hardware domain (i.e dom0). This means 
> that we have to find a way to cooperate with it to keep enable clocks 
> that we care about.
> 
> Regards,
> 
> -- 
> Julien Grall

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

[Stefano Stabellini]

On Thu, 21 Jul 2016, Michael Turquette wrote:
> Quoting Stefano Stabellini (2016-07-14 03:38:04)
> > On Thu, 14 Jul 2016, Dirk Behme wrote:
> > > On 13.07.2016 23:03, Michael Turquette wrote:
> > > > Quoting Dirk Behme (2016-07-13 11:56:30)
> > > > > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > > > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > > > > > On 13.07.2016 00:26, Michael Turquette wrote:
> > > > > > > > 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
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 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.
> > 
> > It might be a step in the right direction, but it doesn't really prevent
> > clk_set_rate from changing properties of a clock owned by Xen.  This
> > patch is incomplete. We need to understand at least what it would take
> > to have a complete solution.
> > 
> > Michael, do you have any suggestions on how it would be possible to set
> > CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
> > way?
> 
> No, there is no way for a consumer to do that. The provider must do it.

All right. But could we design a new device tree binding which the Xen
hypervisor would use to politely ask the clock provider in Linux to set
CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?

Xen would have to modify the DTB before booting Linux with the new
binding.


> > Like you wrote, I would imagine it needs to be done by the clock
> > provider driver. Maybe to do that, it would be easier to have a new
> > device tree property on the clock node, rather than listing phandle and
> > clock-specifier pairs under the Xen node?
> 
> Upon further reflection, I think that your clock consumer can probably
> use clk_set_rate_range() to "lock" in a rate. This is good because it is
> exactly what a clock consumer should do:
> 
> 1) get the clk
> 2) enable the clk
> 3) set the required rate for the clock
> 4) set rate range constraints, or conversely,
> 5) lock in an exact rate; set the min/max rate to the same value
> 
> The problem with this solution is that it requires the consumer to have
> knowledge of the rates that it wants for that clock, which I guess is
> something that Linux kernels in a Xen setup do not want/need?

Who is usually the component with knowledge of the clock rate to set? If
it's a device driver, then neither the Xen hypervisor, nor the Xen core
drivers in Linux would know anything about it. (Unless the clock rate is
specified on device tree via assigned-clock-rates of course.)


> Is it correct that you would prefer some sort of never_touch_this_clk()
> api?

>From my understading, yes, never_touch_this_clk() would make things easier.

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

[Dirk Behme]

Hi Michael, Stefano and Julien,

On 22.07.2016 03:16, Stefano Stabellini wrote:
> On Thu, 21 Jul 2016, Michael Turquette wrote:
>> Quoting Stefano Stabellini (2016-07-14 03:38:04)
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>
>>> It might be a step in the right direction, but it doesn't really prevent
>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>> patch is incomplete. We need to understand at least what it would take
>>> to have a complete solution.
>>>
>>> Michael, do you have any suggestions on how it would be possible to set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
>>> way?
>>
>> No, there is no way for a consumer to do that. The provider must do it.
>
> All right. But could we design a new device tree binding which the Xen
> hypervisor would use to politely ask the clock provider in Linux to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>
> Xen would have to modify the DTB before booting Linux with the new
> binding.
>
>
>>> Like you wrote, I would imagine it needs to be done by the clock
>>> provider driver. Maybe to do that, it would be easier to have a new
>>> device tree property on the clock node, rather than listing phandle and
>>> clock-specifier pairs under the Xen node?
>>
>> Upon further reflection, I think that your clock consumer can probably
>> use clk_set_rate_range() to "lock" in a rate. This is good because it is
>> exactly what a clock consumer should do:
>>
>> 1) get the clk
>> 2) enable the clk
>> 3) set the required rate for the clock
>> 4) set rate range constraints, or conversely,
>> 5) lock in an exact rate; set the min/max rate to the same value
>>
>> The problem with this solution is that it requires the consumer to have
>> knowledge of the rates that it wants for that clock, which I guess is
>> something that Linux kernels in a Xen setup do not want/need?
>
> Who is usually the component with knowledge of the clock rate to set? If
> it's a device driver, then neither the Xen hypervisor, nor the Xen core
> drivers in Linux would know anything about it. (Unless the clock rate is
> specified on device tree via assigned-clock-rates of course.)
>
>
>> Is it correct that you would prefer some sort of never_touch_this_clk()
>> api?
>
>>From my understading, yes, never_touch_this_clk() would make things easier.


Would it be somehow worth to wait for anything like this 
never_touch_this_clk() api? Or should we try to proceed with 
clk_prepare_enable() like done in this patch for the moment?

Best regards

Dirk

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

[Julien Grall]

Hi Dirk,

On 27/07/16 06:05, Dirk Behme wrote:
> Hi Michael, Stefano and Julien,
>
> On 22.07.2016 03:16, Stefano Stabellini wrote:
>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>> Quoting Stefano Stabellini (2016-07-14 03:38:04)
>>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>
>>>> It might be a step in the right direction, but it doesn't really
>>>> prevent
>>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>>> patch is incomplete. We need to understand at least what it would take
>>>> to have a complete solution.
>>>>
>>>> Michael, do you have any suggestions on how it would be possible to set
>>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
>>>> way?
>>>
>>> No, there is no way for a consumer to do that. The provider must do it.
>>
>> All right. But could we design a new device tree binding which the Xen
>> hypervisor would use to politely ask the clock provider in Linux to set
>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>
>> Xen would have to modify the DTB before booting Linux with the new
>> binding.
>>
>>
>>>> Like you wrote, I would imagine it needs to be done by the clock
>>>> provider driver. Maybe to do that, it would be easier to have a new
>>>> device tree property on the clock node, rather than listing phandle and
>>>> clock-specifier pairs under the Xen node?
>>>
>>> Upon further reflection, I think that your clock consumer can probably
>>> use clk_set_rate_range() to "lock" in a rate. This is good because it is
>>> exactly what a clock consumer should do:
>>>
>>> 1) get the clk
>>> 2) enable the clk
>>> 3) set the required rate for the clock
>>> 4) set rate range constraints, or conversely,
>>> 5) lock in an exact rate; set the min/max rate to the same value
>>>
>>> The problem with this solution is that it requires the consumer to have
>>> knowledge of the rates that it wants for that clock, which I guess is
>>> something that Linux kernels in a Xen setup do not want/need?
>>
>> Who is usually the component with knowledge of the clock rate to set? If
>> it's a device driver, then neither the Xen hypervisor, nor the Xen core
>> drivers in Linux would know anything about it. (Unless the clock rate is
>> specified on device tree via assigned-clock-rates of course.)
>>
>>
>>> Is it correct that you would prefer some sort of never_touch_this_clk()
>>> api?
>>
>>> From my understading, yes, never_touch_this_clk() would make things
>>> easier.
>
>
> Would it be somehow worth to wait for anything like this
> never_touch_this_clk() api? Or should we try to proceed with
> clk_prepare_enable() like done in this patch for the moment?

I am not sure who will write the new api never_touch_this_clk(). Could 
you suggest an implementation based on the discussion?

Regards,

>
> Best regards
>
> Dirk
>

-- 
Julien Grall

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

[Dirk Behme]

On 28.07.2016 13:17, Julien Grall wrote:
> Hi Dirk,
>
> On 27/07/16 06:05, Dirk Behme wrote:
>> Hi Michael, Stefano and Julien,
>>
>> On 22.07.2016 03:16, Stefano Stabellini wrote:
>>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>>> Quoting Stefano Stabellini (2016-07-14 03:38:04)
>>>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>
>>>>> It might be a step in the right direction, but it doesn't really
>>>>> prevent
>>>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>>>> patch is incomplete. We need to understand at least what it would
>>>>> take
>>>>> to have a complete solution.
>>>>>
>>>>> Michael, do you have any suggestions on how it would be possible
>>>>> to set
>>>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a
>>>>> proper
>>>>> way?
>>>>
>>>> No, there is no way for a consumer to do that. The provider must
>>>> do it.
>>>
>>> All right. But could we design a new device tree binding which the Xen
>>> hypervisor would use to politely ask the clock provider in Linux to
>>> set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>>
>>> Xen would have to modify the DTB before booting Linux with the new
>>> binding.
>>>
>>>
>>>>> Like you wrote, I would imagine it needs to be done by the clock
>>>>> provider driver. Maybe to do that, it would be easier to have a new
>>>>> device tree property on the clock node, rather than listing
>>>>> phandle and
>>>>> clock-specifier pairs under the Xen node?
>>>>
>>>> Upon further reflection, I think that your clock consumer can
>>>> probably
>>>> use clk_set_rate_range() to "lock" in a rate. This is good because
>>>> it is
>>>> exactly what a clock consumer should do:
>>>>
>>>> 1) get the clk
>>>> 2) enable the clk
>>>> 3) set the required rate for the clock
>>>> 4) set rate range constraints, or conversely,
>>>> 5) lock in an exact rate; set the min/max rate to the same value
>>>>
>>>> The problem with this solution is that it requires the consumer to
>>>> have
>>>> knowledge of the rates that it wants for that clock, which I guess is
>>>> something that Linux kernels in a Xen setup do not want/need?
>>>
>>> Who is usually the component with knowledge of the clock rate to
>>> set? If
>>> it's a device driver, then neither the Xen hypervisor, nor the Xen
>>> core
>>> drivers in Linux would know anything about it. (Unless the clock
>>> rate is
>>> specified on device tree via assigned-clock-rates of course.)
>>>
>>>
>>>> Is it correct that you would prefer some sort of
>>>> never_touch_this_clk()
>>>> api?
>>>
>>>> From my understading, yes, never_touch_this_clk() would make things
>>>> easier.
>>
>>
>> Would it be somehow worth to wait for anything like this
>> never_touch_this_clk() api? Or should we try to proceed with
>> clk_prepare_enable() like done in this patch for the moment?
>
> I am not sure who will write the new api never_touch_this_clk(). Could
> you suggest an implementation based on the discussion?


As this was a proposal from Michael, I'm hoping for Michael here, 
somehow ;) At least for a hint if anything like never_touch_this_clk() 
would be realistic to get accepted. And if so, how this could look like.

If this is unrealistic, I think we should go the proposed 
clk_prepare_enable() way, as it seems this is the best we could do at 
the moment without never_touch_this_clk().

Best regards

Dirk