Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 08/07/16 07:48, Dirk Behme wrote:
#include
#include
#include
@@ -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);
Is there a struct device we can use here?
It doesn't look so. Julien?
We don't have a struct device. Maybe we can create a dummy one if it
simplifies the logic?
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi Dirk, On 08/07/16 06:51, Dirk Behme wrote: On 08.07.2016 04:50, Michael Turquette wrote: Quoting Dirk Behme (2016-07-07 00:32:34) The two issues to resolve are: 1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model? I think so. Julien, what do you think? I don't think it fits the Xen model. I will give more details on the v3 as your commit message give an example with is contradictory to the model. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi,
On 06.07.2016 03:34, Michael Turquette wrote:
Hi!
Quoting Dirk Behme (2016-06-30 03:32:32)
Some clocks might be used by the Xen hypervisor and not by the Linux
kernel. If these are not registered by the Linux kernel, they might be
disabled by clk_disable_unused() as the kernel doesn't know that they
are used. The clock of the serial console handled by Xen is one
example for this. It might be disabled by clk_disable_unused() which
stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled
down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and
enables them, which is what every other clk consumer in the Linux kernel
does. More details below.
Up to now, the workaround for this has been to use the Linux kernel
command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting
that flag will not turn clocks on for you, nor will it guarantee that
those clocks are never turned off in the future. It looks like you
figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that
flag only exists as a way to enable clocks that must be enabled for the
system to function (hence, "critical") AND when those same clocks do not
have an accompanying Linux driver to consume them and enable them.
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor
node. The Linux kernel has to register the clocks from the hypervisor
node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node
and if so register the given clocks to the Linux kernel clock
framework and with this mark them as used. This prevents the clocks
from being disabled.
Signed-off-by: Dirk Behme
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
Documentation/devicetree/bindings/arm/xen.txt | 11 +++
arch/arm/xen/enlighten.c | 47 +++
2 files changed, 58 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/xen.txt
b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..55dfd3b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -17,6 +17,17 @@ the following properties:
A GIC node is also required.
This property is unnecessary when booting Dom0 using ACPI.
+Optional properties:
+
+- clocks: one or more clocks to be registered.
s/registered/consumed/
For appropriate DT binding script to steal I picked one at random:
Documentation/devicetree/bindings/clock/ti/clockdomain.txt
+ Xen hypervisor drivers might replace native drivers, resulting in
+ clocks not registered by these native drivers. To avoid that these
+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+ register them in the hypervisor node.
+ An example for this are the clocks of the serial driver. If the clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.
+
To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
under /hypervisor with following parameters:
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
#include
#include
+#include
s/clk-provider.h/clk.h/
clk-provider.h is only used for providers and this bit of code is a
consumer.
It seems we need clk-provider.h for of_clk_get_parent_count()?
#include
#include
#include
@@ -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);
Is there a struct device we can use here?
It doesn't look so. Julien?
It would be better to use
devm_clk_get if possible.
Best regards
Dirk
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.x
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 08.07.2016 04:50, Michael Turquette wrote: Quoting Dirk Behme (2016-07-07 00:32:34) Hi Michael, On 06.07.2016 22:42, Michael Turquette wrote: Hi Julien, Quoting Julien Grall (2016-07-06 06:10:52) On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused. Maybe we need some advice here :) Sure! I've used clk_prepare_enable() 'just' to get the enable count incremented clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns. http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751 Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock. Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm). The Sneaky Sibling Attack(tm) occurs when clock A and clock B are siblings and share the same parent, clock C. If clock A is enabled in hardware (by bootloader, firmware or hypervisor), but does NOT have a positive enable_count (in Linux), then it is possible that a driver might call clk_enable(clk_B) then clk_disable(clk_B), which will result in the disable action propagating up the parent chain and disabling clk_C, the shared parent. This will of course gate clk_A, which is clocked by clk_C, breaking things for you. So you need to be worried about more than just clk_disable_unused. The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable. Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy. If there is an other / better / correct way to achieve that, please let us know. Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use". I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?) Right, the flags are immutable and must be set by the clock provider driver before registering the clock. Toggling flags at run-time is a misuse of the flags, and clock consumer drivers should never care about the flags. They are internal to the clock framework. In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them. Ok, thanks! The two issues to resolve are: 1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model? I think so. Julien, what do you think? 2) the language of the binding description makes this way more confusing than it needs to be. Just claim the resources you need and enable them, which is an OS-agnostic action. I'll post a v3 patch trying to update the description with your proposals. Best regards Dirk ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Quoting Dirk Behme (2016-07-07 00:32:34) > Hi Michael, > > On 06.07.2016 22:42, Michael Turquette wrote: > > Hi Julien, > > > > Quoting Julien Grall (2016-07-06 06:10:52) > >> On 06/07/16 02:34, Michael Turquette wrote: > >>> Hi! > >> > >> Hello Michael, > >> > >>> Quoting Dirk Behme (2016-06-30 03:32:32) > Some clocks might be used by the Xen hypervisor and not by the Linux > kernel. If these are not registered by the Linux kernel, they might be > disabled by clk_disable_unused() as the kernel doesn't know that they > are used. The clock of the serial console handled by Xen is one > example for this. It might be disabled by clk_disable_unused() which > stops the whole serial output, even from Xen, then. > >>> > >>> This whole thread had me confused until I realized that it all boiled > >>> down to some nomenclature issues (for me). > >>> > >>> This code does not _register_ any clocks. It simply gets them and > >>> enables them, which is what every other clk consumer in the Linux kernel > >>> does. More details below. > >>> > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 > >>> > >>> clk_ignore_unused is a band-aid, not a proper medical solution. Setting > >>> that flag will not turn clocks on for you, nor will it guarantee that > >>> those clocks are never turned off in the future. It looks like you > >>> figured this out correctly in the patch below but it is worth repeating. > >>> > >>> Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > >>> flag only exists as a way to enable clocks that must be enabled for the > >>> system to function (hence, "critical") AND when those same clocks do not > >>> have an accompanying Linux driver to consume them and enable them. > >> > >> I don't think we want the kernel to enable the clock for the hypervisor. > >> We want to tell the kernel "don't touch at all to this clock, it does > >> not belong to you". > > > > But the patch *does* touch the clock from the kernel. It enables the > > clock via a call clk_prepare_enable. I'm utterly confused. > > > Maybe we need some advice here :) Sure! > > > I've used clk_prepare_enable() 'just' to get the enable count incremented clk_prepare_enabled will *enable* the clock signal if it currently disabled or gated. In other words, if the physical line is not toggling before the call, it will be after the call returns. > > http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751 > > Because it's my understanding that enable_count is needed to prevent > clk_disable_unused() from disabling the clock. Having a positive enable_count will prevent the clock from being disabled by both clk_disable_unused AND from the Sneaky Sibling Attack(tm). The Sneaky Sibling Attack(tm) occurs when clock A and clock B are siblings and share the same parent, clock C. If clock A is enabled in hardware (by bootloader, firmware or hypervisor), but does NOT have a positive enable_count (in Linux), then it is possible that a driver might call clk_enable(clk_B) then clk_disable(clk_B), which will result in the disable action propagating up the parent chain and disabling clk_C, the shared parent. This will of course gate clk_A, which is clocked by clk_C, breaking things for you. So you need to be worried about more than just clk_disable_unused. The simple fact is is that if a piece of software knows that it needs for its clock to be enabled, it should actively enable it with clk_prepare_enable. Doing some weird stuff with CLK_IGNORE_UNUSED or anything else is just hoping that your clock will not be disabled, and that is the wrong strategy. > > > If there is an other / better / correct way to achieve that, please let > us know. Well, you should not "try to prevent a clock from being disabled", you should "enable the clock that you need to use". > > > I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't > find a function exported by the clock framework to set that flag (?) Right, the flags are immutable and must be set by the clock provider driver before registering the clock. Toggling flags at run-time is a misuse of the flags, and clock consumer drivers should never care about the flags. They are internal to the clock framework. In conclusion, I think that your patch does the right thing. The Xen node consumes the clocks that it needs to manage and it calls clk_prepare_enable on them. The two issues to resolve are: 1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model? 2) the language of the binding description makes this way more confusing than it needs to be. Just claim the resources you need and enable them, which is an OS-agnostic action. Regards, Mike > > > Best regards > > Dirk ___ Xen-devel mailing list Xen-devel@lists.
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi Michael, On 06.07.2016 22:42, Michael Turquette wrote: Hi Julien, Quoting Julien Grall (2016-07-06 06:10:52) On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused. Maybe we need some advice here :) I've used clk_prepare_enable() 'just' to get the enable count incremented http://lxr.free-electrons.com/source/drivers/clk/clk.c#L751 Because it's my understanding that enable_count is needed to prevent clk_disable_unused() from disabling the clock. If there is an other / better / correct way to achieve that, please let us know. I've had a look to use the CLK_IGNORE_UNUSED flag, too. But couldn't find a function exported by the clock framework to set that flag (?) Best regards Dirk ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Quoting Mark Rutland (2016-07-05 04:07:37) > Hi, > > On Tue, Jul 05, 2016 at 12:45:34PM +0200, Dirk Behme wrote: > > On 05.07.2016 12:39, Mark Rutland wrote: > > >On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote: > > >>+- clocks: one or more clocks to be registered. > > >>+ Xen hypervisor drivers might replace native drivers, resulting in > > >>+ clocks not registered by these native drivers. To avoid that these > > >>+ unregistered clocks are disabled by the Linux kernel initialization > > >>+ register them in the hypervisor node. > > >>+ An example for this are the clocks of a serial driver already enabled > > >>+ by the firmware. If the clocks used by the serial hardware interface > > >>+ are not registered by the serial driver itself the serial output > > >>+ might stop once the Linux kernel initialization disables the 'unused' > > >>+ clocks. > > > > > >The above describes the set of problems, but doesn't set out the actual > > >contract. It also covers a number of Linux implementation details in > > >abstract. > > > > Could you kindly be a little more specific which 'implementation > > details' you don't like? > > The fact that we disable some clocks at init time is a driver model > thing that depends on various factors (e.g. cmdline options), and it's > something that could be moved around. We only mention disabling, and not > rate change (which could happen, even if it doesn't today). > > I don't think that we need to describe the Linux behaviour at all. > > > E.g. to my understanding, the 'implementation detail' that Linux > > disables unregistered clocks is needed for the description. > > > > If you have a different wording in mind, could you kindly share that? > > Something like: > > - 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. Fully agree that we should forget the clk_disable_unused stuff entirely. Mark's copy above is good, and just for the fun of it I have provided an alternative version. Feel free to pick and choose what you want: - clocks: one or more clocks to be enabled. Xen hypervisor drivers might replace native OS drivers. The result is that some important clocks that are enabled by the OS in the non-Xen case are not properly enabled in the presence of Xen. The clocks property enumerates the clocks that must be enabled by the Xen clock consumer. An example is a serial driver enabled by the hypervisor. Xen must consume and enable these clocks in the OS to ensure behavior continues after firmware configures the UART hardware and corresponding clock harder. Regards, Mike > > > >As I commented previously [1], the binding should describe the set of > > >guarantees that you rewquire (e.g. that the clocks must be left as-is, > > >not gated, and their rates left unchanged). > > > > > >Please describe the specific set of guarantees that you require. > > > > To my understanding this is done, already: "avoid that these ... > > clocks are disabled" > > My point of contention here is that while this might tell a dts author > what to place in this property, it doesn't specify what the OS should > do. > > Thanks, > Mark. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi Julien, Quoting Julien Grall (2016-07-06 06:10:52) > On 06/07/16 02:34, Michael Turquette wrote: > > Hi! > > Hello Michael, > > > Quoting Dirk Behme (2016-06-30 03:32:32) > >> Some clocks might be used by the Xen hypervisor and not by the Linux > >> kernel. If these are not registered by the Linux kernel, they might be > >> disabled by clk_disable_unused() as the kernel doesn't know that they > >> are used. The clock of the serial console handled by Xen is one > >> example for this. It might be disabled by clk_disable_unused() which > >> stops the whole serial output, even from Xen, then. > > > > This whole thread had me confused until I realized that it all boiled > > down to some nomenclature issues (for me). > > > > This code does not _register_ any clocks. It simply gets them and > > enables them, which is what every other clk consumer in the Linux kernel > > does. More details below. > > > >> > >> Up to now, the workaround for this has been to use the Linux kernel > >> command line parameter 'clk_ignore_unused'. See Xen bug > >> > >> http://bugs.xenproject.org/xen/bug/45 > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting > > that flag will not turn clocks on for you, nor will it guarantee that > > those clocks are never turned off in the future. It looks like you > > figured this out correctly in the patch below but it is worth repeating. > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > > flag only exists as a way to enable clocks that must be enabled for the > > system to function (hence, "critical") AND when those same clocks do not > > have an accompanying Linux driver to consume them and enable them. > > I don't think we want the kernel to enable the clock for the hypervisor. > We want to tell the kernel "don't touch at all to this clock, it does > not belong to you". But the patch *does* touch the clock from the kernel. It enables the clock via a call clk_prepare_enable. I'm utterly confused. Regards, Mike > > Regards, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Wed, Jul 06, 2016 at 02:16:18PM +0100, Stefano Stabellini wrote: > On Wed, 6 Jul 2016, Julien Grall wrote: > > On 06/07/16 02:34, Michael Turquette wrote: > > > Hi! > > > > Hello Michael, > > > > > Quoting Dirk Behme (2016-06-30 03:32:32) > > > > Some clocks might be used by the Xen hypervisor and not by the Linux > > > > kernel. If these are not registered by the Linux kernel, they might be > > > > disabled by clk_disable_unused() as the kernel doesn't know that they > > > > are used. The clock of the serial console handled by Xen is one > > > > example for this. It might be disabled by clk_disable_unused() which > > > > stops the whole serial output, even from Xen, then. > > > > > > This whole thread had me confused until I realized that it all boiled > > > down to some nomenclature issues (for me). > > > > > > This code does not _register_ any clocks. It simply gets them and > > > enables them, which is what every other clk consumer in the Linux kernel > > > does. More details below. > > > > > > > > > > > Up to now, the workaround for this has been to use the Linux kernel > > > > command line parameter 'clk_ignore_unused'. See Xen bug > > > > > > > > http://bugs.xenproject.org/xen/bug/45 > > > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting > > > that flag will not turn clocks on for you, nor will it guarantee that > > > those clocks are never turned off in the future. It looks like you > > > figured this out correctly in the patch below but it is worth repeating. > > > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > > > flag only exists as a way to enable clocks that must be enabled for the > > > system to function (hence, "critical") AND when those same clocks do not > > > have an accompanying Linux driver to consume them and enable them. > > > > I don't think we want the kernel to enable the clock for the hypervisor. We > > want to tell the kernel "don't touch at all to this clock, it does not > > belong > > to you". > > Right, and that's why I was suggesting that another way to do this would > be to set the "status" to "disabled" in Xen: so that Linux would leave > the clock alone. But in that case Linux would not be happy to see > disabled clocks which are actually supposed to be used by some devices. If you were to do that, that would cover the entire clock-controller, not necessarily for the individual clock line (as this does not necessarily have a node of its own). So you'd prevent the use of other clocks owned by that controller. That's also not sufficient, as you'd have to do the same for resources required to keep that clock active (parent clocks from different controllers, regulators, GPIOs, etc). I don't think that will work other than in very basic cases. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 06/07/16 14:16, Stefano Stabellini wrote: On Wed, 6 Jul 2016, Julien Grall wrote: On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct? The problem is not "whether Linux would be happy or not", but the meaning of this property. Based on the ePAPR, "status = disabled" indicates that the device is not presently operational, but it might become operational later (for example, something is not plugged in, or switched off). An operating system which read this property should interpret as "the clock should not be used". However, with your suggestion the OS would need to differentiate between "the clock may be used by the hypervisor" and "the clock is not wired up/present/...". Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Wed, 6 Jul 2016, Julien Grall wrote: > On 06/07/16 02:34, Michael Turquette wrote: > > Hi! > > Hello Michael, > > > Quoting Dirk Behme (2016-06-30 03:32:32) > > > Some clocks might be used by the Xen hypervisor and not by the Linux > > > kernel. If these are not registered by the Linux kernel, they might be > > > disabled by clk_disable_unused() as the kernel doesn't know that they > > > are used. The clock of the serial console handled by Xen is one > > > example for this. It might be disabled by clk_disable_unused() which > > > stops the whole serial output, even from Xen, then. > > > > This whole thread had me confused until I realized that it all boiled > > down to some nomenclature issues (for me). > > > > This code does not _register_ any clocks. It simply gets them and > > enables them, which is what every other clk consumer in the Linux kernel > > does. More details below. > > > > > > > > Up to now, the workaround for this has been to use the Linux kernel > > > command line parameter 'clk_ignore_unused'. See Xen bug > > > > > > http://bugs.xenproject.org/xen/bug/45 > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting > > that flag will not turn clocks on for you, nor will it guarantee that > > those clocks are never turned off in the future. It looks like you > > figured this out correctly in the patch below but it is worth repeating. > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > > flag only exists as a way to enable clocks that must be enabled for the > > system to function (hence, "critical") AND when those same clocks do not > > have an accompanying Linux driver to consume them and enable them. > > I don't think we want the kernel to enable the clock for the hypervisor. We > want to tell the kernel "don't touch at all to this clock, it does not belong > to you". Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi!
Quoting Dirk Behme (2016-06-30 03:32:32)
> Some clocks might be used by the Xen hypervisor and not by the Linux
> kernel. If these are not registered by the Linux kernel, they might be
> disabled by clk_disable_unused() as the kernel doesn't know that they
> are used. The clock of the serial console handled by Xen is one
> example for this. It might be disabled by clk_disable_unused() which
> stops the whole serial output, even from Xen, then.
This whole thread had me confused until I realized that it all boiled
down to some nomenclature issues (for me).
This code does not _register_ any clocks. It simply gets them and
enables them, which is what every other clk consumer in the Linux kernel
does. More details below.
>
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
>
> http://bugs.xenproject.org/xen/bug/45
clk_ignore_unused is a band-aid, not a proper medical solution. Setting
that flag will not turn clocks on for you, nor will it guarantee that
those clocks are never turned off in the future. It looks like you
figured this out correctly in the patch below but it is worth repeating.
Also the new CLK_IS_CRITICAL flag might be of interest to you, but that
flag only exists as a way to enable clocks that must be enabled for the
system to function (hence, "critical") AND when those same clocks do not
have an accompanying Linux driver to consume them and enable them.
>
> too.
>
> To fix this, we will add the "unused" clocks in Xen to the hypervisor
> node. The Linux kernel has to register the clocks from the hypervisor
> node, then.
>
> Therefore, check if there is a "clocks" entry in the hypervisor node
> and if so register the given clocks to the Linux kernel clock
> framework and with this mark them as used. This prevents the clocks
> from being disabled.
>
> Signed-off-by: Dirk Behme
> ---
> Changes in v2:
> - Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
> for-linus-4.8
> - Add changes to Documentation/devicetree/bindings/arm/xen.txt
>
> Documentation/devicetree/bindings/arm/xen.txt | 11 +++
> arch/arm/xen/enlighten.c | 47
> +++
> 2 files changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> b/Documentation/devicetree/bindings/arm/xen.txt
> index c9b9321..55dfd3b 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -17,6 +17,17 @@ the following properties:
>A GIC node is also required.
>This property is unnecessary when booting Dom0 using ACPI.
>
> +Optional properties:
> +
> +- clocks: one or more clocks to be registered.
s/registered/consumed/
For appropriate DT binding script to steal I picked one at random:
Documentation/devicetree/bindings/clock/ti/clockdomain.txt
> + Xen hypervisor drivers might replace native drivers, resulting in
> + clocks not registered by these native drivers. To avoid that these
> + unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
> + register them in the hypervisor node.
> + An example for this are the clocks of the serial driver. If the clocks
> + used by the serial hardware interface are not registered by the serial
> + driver the serial output might stop once clk_disable_unused() is called.
> +
> To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi"
> node
> under /hypervisor with following parameters:
>
> 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
> #include
> #include
> +#include
s/clk-provider.h/clk.h/
clk-provider.h is only used for providers and this bit of code is a
consumer.
> #include
> #include
> #include
> @@ -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);
Is there a struct device we can use here? It would be better to use
devm_clk_get if possible.
Regards,
Mike
> + if (IS_ERR(clk)) {
> + pr_err("Xen fa
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Tue, 5 Jul 2016, Julien Grall wrote: > On 05/07/16 15:04, Stefano Stabellini wrote: > > On Tue, 5 Jul 2016, Julien Grall wrote: > > > On 05/07/16 14:53, Stefano Stabellini wrote: > > > > On Thu, 30 Jun 2016, Dirk Behme wrote: > > > > > +- clocks: one or more clocks to be registered. > > > > > + Xen hypervisor drivers might replace native drivers, resulting in > > > > > + clocks not registered by these native drivers. To avoid that these > > > > > + unregistered clocks are disabled, then, e.g. by > > > > > clk_disable_unused(), > > > > > + register them in the hypervisor node. > > > > > + An example for this are the clocks of the serial driver. If the > > > > > clocks > > > > > + used by the serial hardware interface are not registered by the > > > > > serial > > > > > + driver the serial output might stop once clk_disable_unused() is > > > > > called. > > > > > > > > What if we use the "status" property of the clocks? Could we set it to > > > > "disabled" in Xen? Would that be enough for Linux to leave them alone? > > > > > > clocks could be shared between multiple devices. So it is not possible to > > > disable the clock. > > > > To clarify my suggestion: I am not saying we should disable the clock, I > > am saying we should set the "status" property to "disabled" in Xen for > > the clock used by the serial or passthrough devices (for which the > > "status" property is already set to "disabled"). That should work for > > cases where the clock is not shared among multiple devices. > > How would you be able to differentiate in Xen between a clock shared and a > non-shared one? The only way I can think it going through all the device tree > which sounds really expensive. Yes, but it is only done once and we only need to do the search for clocks of "disabled" devices. > > If the clock is shared, then I don't think we would run into the issue > > described by Dirk because I wouldn't imagine clk_disable_unused would > > try to disable the clock anymore, because it would actually be in use > > from Linux POV. > > We also want to prevent Linux changing the rate of the clock (see Mark's mail > [1]). I am not sure what's the best way to achieve that for clocks shared across multiple devices. Let's see what the clock maintainers suggest. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 05/07/16 15:04, Stefano Stabellini wrote: On Tue, 5 Jul 2016, Julien Grall wrote: On 05/07/16 14:53, Stefano Stabellini wrote: On Thu, 30 Jun 2016, Dirk Behme wrote: +- clocks: one or more clocks to be registered. + Xen hypervisor drivers might replace native drivers, resulting in + clocks not registered by these native drivers. To avoid that these + unregistered clocks are disabled, then, e.g. by clk_disable_unused(), + register them in the hypervisor node. + An example for this are the clocks of the serial driver. If the clocks + used by the serial hardware interface are not registered by the serial + driver the serial output might stop once clk_disable_unused() is called. What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone? clocks could be shared between multiple devices. So it is not possible to disable the clock. To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices. How would you be able to differentiate in Xen between a clock shared and a non-shared one? The only way I can think it going through all the device tree which sounds really expensive. If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV. We also want to prevent Linux changing the rate of the clock (see Mark's mail [1]). Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00335.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Tue, 5 Jul 2016, Julien Grall wrote: > On 05/07/16 14:53, Stefano Stabellini wrote: > > On Thu, 30 Jun 2016, Dirk Behme wrote: > > > +- clocks: one or more clocks to be registered. > > > + Xen hypervisor drivers might replace native drivers, resulting in > > > + clocks not registered by these native drivers. To avoid that these > > > + unregistered clocks are disabled, then, e.g. by clk_disable_unused(), > > > + register them in the hypervisor node. > > > + An example for this are the clocks of the serial driver. If the clocks > > > + used by the serial hardware interface are not registered by the serial > > > + driver the serial output might stop once clk_disable_unused() is > > > called. > > > > What if we use the "status" property of the clocks? Could we set it to > > "disabled" in Xen? Would that be enough for Linux to leave them alone? > > clocks could be shared between multiple devices. So it is not possible to > disable the clock. To clarify my suggestion: I am not saying we should disable the clock, I am saying we should set the "status" property to "disabled" in Xen for the clock used by the serial or passthrough devices (for which the "status" property is already set to "disabled"). That should work for cases where the clock is not shared among multiple devices. If the clock is shared, then I don't think we would run into the issue described by Dirk because I wouldn't imagine clk_disable_unused would try to disable the clock anymore, because it would actually be in use from Linux POV. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 05/07/16 14:54, Julien Grall wrote:
On 05/07/16 14:53, Stefano Stabellini wrote:
On Thu, 30 Jun 2016, Dirk Behme wrote:
+- clocks: one or more clocks to be registered.
+ Xen hypervisor drivers might replace native drivers, resulting in
+ clocks not registered by these native drivers. To avoid that these
+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+ register them in the hypervisor node.
+ An example for this are the clocks of the serial driver. If the
clocks
+ used by the serial hardware interface are not registered by the
serial
+ driver the serial output might stop once clk_disable_unused() is
called.
What if we use the "status" property of the clocks? Could we set it to
"disabled" in Xen? Would that be enough for Linux to leave them alone?
clocks could be shared between multiple devices. So it is not possible
to disable the clock.
To give an example, below the UART node for juno
(arch/arm64/boot/dts/arm/juno-base.dtsi):
soc_uart0: uart@7ff8 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0 0x7ff8 0x0 0x1000>;
interrupts = ;
clocks = <&soc_uartclk>, <&soc_refclk100mhz>;
clock-names = "uartclk", "apb_pclk";
};
The clock soc_refclk100mhz is shared with the mailbox.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 05/07/16 14:53, Stefano Stabellini wrote: On Thu, 30 Jun 2016, Dirk Behme wrote: +- clocks: one or more clocks to be registered. + Xen hypervisor drivers might replace native drivers, resulting in + clocks not registered by these native drivers. To avoid that these + unregistered clocks are disabled, then, e.g. by clk_disable_unused(), + register them in the hypervisor node. + An example for this are the clocks of the serial driver. If the clocks + used by the serial hardware interface are not registered by the serial + driver the serial output might stop once clk_disable_unused() is called. What if we use the "status" property of the clocks? Could we set it to "disabled" in Xen? Would that be enough for Linux to leave them alone? clocks could be shared between multiple devices. So it is not possible to disable the clock. -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Thu, 30 Jun 2016, Dirk Behme wrote:
> Some clocks might be used by the Xen hypervisor and not by the Linux
> kernel. If these are not registered by the Linux kernel, they might be
> disabled by clk_disable_unused() as the kernel doesn't know that they
> are used. The clock of the serial console handled by Xen is one
> example for this. It might be disabled by clk_disable_unused() which
> stops the whole serial output, even from Xen, then.
>
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
>
> http://bugs.xenproject.org/xen/bug/45
>
> too.
>
> To fix this, we will add the "unused" clocks in Xen to the hypervisor
> node. The Linux kernel has to register the clocks from the hypervisor
> node, then.
>
> Therefore, check if there is a "clocks" entry in the hypervisor node
> and if so register the given clocks to the Linux kernel clock
> framework and with this mark them as used. This prevents the clocks
> from being disabled.
>
> Signed-off-by: Dirk Behme
> ---
> Changes in v2:
> - Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
> for-linus-4.8
> - Add changes to Documentation/devicetree/bindings/arm/xen.txt
>
> Documentation/devicetree/bindings/arm/xen.txt | 11 +++
> arch/arm/xen/enlighten.c | 47
> +++
> 2 files changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> b/Documentation/devicetree/bindings/arm/xen.txt
> index c9b9321..55dfd3b 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -17,6 +17,17 @@ the following properties:
>A GIC node is also required.
>This property is unnecessary when booting Dom0 using ACPI.
>
> +Optional properties:
> +
> +- clocks: one or more clocks to be registered.
> + Xen hypervisor drivers might replace native drivers, resulting in
> + clocks not registered by these native drivers. To avoid that these
> + unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
> + register them in the hypervisor node.
> + An example for this are the clocks of the serial driver. If the clocks
> + used by the serial hardware interface are not registered by the serial
> + driver the serial output might stop once clk_disable_unused() is called.
What if we use the "status" property of the clocks? Could we set it to
"disabled" in Xen? Would that be enough for Linux to leave them alone?
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi, On Tue, Jul 05, 2016 at 12:45:34PM +0200, Dirk Behme wrote: > On 05.07.2016 12:39, Mark Rutland wrote: > >On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote: > >>+- clocks: one or more clocks to be registered. > >>+ Xen hypervisor drivers might replace native drivers, resulting in > >>+ clocks not registered by these native drivers. To avoid that these > >>+ unregistered clocks are disabled by the Linux kernel initialization > >>+ register them in the hypervisor node. > >>+ An example for this are the clocks of a serial driver already enabled > >>+ by the firmware. If the clocks used by the serial hardware interface > >>+ are not registered by the serial driver itself the serial output > >>+ might stop once the Linux kernel initialization disables the 'unused' > >>+ clocks. > > > >The above describes the set of problems, but doesn't set out the actual > >contract. It also covers a number of Linux implementation details in > >abstract. > > Could you kindly be a little more specific which 'implementation > details' you don't like? The fact that we disable some clocks at init time is a driver model thing that depends on various factors (e.g. cmdline options), and it's something that could be moved around. We only mention disabling, and not rate change (which could happen, even if it doesn't today). I don't think that we need to describe the Linux behaviour at all. > E.g. to my understanding, the 'implementation detail' that Linux > disables unregistered clocks is needed for the description. > > If you have a different wording in mind, could you kindly share that? Something like: - 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. > >As I commented previously [1], the binding should describe the set of > >guarantees that you rewquire (e.g. that the clocks must be left as-is, > >not gated, and their rates left unchanged). > > > >Please describe the specific set of guarantees that you require. > > To my understanding this is done, already: "avoid that these ... > clocks are disabled" My point of contention here is that while this might tell a dts author what to place in this property, it doesn't specify what the OS should do. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi Mark, On 05.07.2016 12:39, Mark Rutland wrote: Hi, On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote: Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 too. To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then. Therefore, check if there is a "clocks" entry in the hypervisor node and if so register the given clocks to the Linux kernel clock framework and with this mark them as used. This prevents the clocks from being disabled. Signed-off-by: Dirk Behme --- Changes in v2: Drop the Linux implementation details like clk_disable_unused in xen.txt. Thanks for doing this. Documentation/devicetree/bindings/arm/xen.txt | 13 arch/arm/xen/enlighten.c | 47 +++ 2 files changed, 60 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt index c9b9321..21fd469 100644 --- a/Documentation/devicetree/bindings/arm/xen.txt +++ b/Documentation/devicetree/bindings/arm/xen.txt @@ -17,6 +17,19 @@ the following properties: A GIC node is also required. This property is unnecessary when booting Dom0 using ACPI. +Optional properties: + +- clocks: one or more clocks to be registered. + Xen hypervisor drivers might replace native drivers, resulting in + clocks not registered by these native drivers. To avoid that these + unregistered clocks are disabled by the Linux kernel initialization + register them in the hypervisor node. + An example for this are the clocks of a serial driver already enabled + by the firmware. If the clocks used by the serial hardware interface + are not registered by the serial driver itself the serial output + might stop once the Linux kernel initialization disables the 'unused' + clocks. The above describes the set of problems, but doesn't set out the actual contract. It also covers a number of Linux implementation details in abstract. Could you kindly be a little more specific which 'implementation details' you don't like? E.g. to my understanding, the 'implementation detail' that Linux disables unregistered clocks is needed for the description. If you have a different wording in mind, could you kindly share that? As I commented previously [1], the binding should describe the set of guarantees that you rewquire (e.g. that the clocks must be left as-is, not gated, and their rates left unchanged). Please describe the specific set of guarantees that you require. To my understanding this is done, already: "avoid that these ... clocks are disabled" Best regards Dirk ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi, On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote: > Some clocks might be used by the Xen hypervisor and not by the Linux > kernel. If these are not registered by the Linux kernel, they might be > disabled by clk_disable_unused() as the kernel doesn't know that they > are used. The clock of the serial console handled by Xen is one > example for this. It might be disabled by clk_disable_unused() which > stops the whole serial output, even from Xen, then. > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 > > too. > > To fix this, we will add the "unused" clocks in Xen to the hypervisor > node. The Linux kernel has to register the clocks from the hypervisor > node, then. > > Therefore, check if there is a "clocks" entry in the hypervisor node > and if so register the given clocks to the Linux kernel clock > framework and with this mark them as used. This prevents the clocks > from being disabled. > > Signed-off-by: Dirk Behme > --- > Changes in v2: Drop the Linux implementation details like clk_disable_unused > in xen.txt. Thanks for doing this. > Documentation/devicetree/bindings/arm/xen.txt | 13 > arch/arm/xen/enlighten.c | 47 > +++ > 2 files changed, 60 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/xen.txt > b/Documentation/devicetree/bindings/arm/xen.txt > index c9b9321..21fd469 100644 > --- a/Documentation/devicetree/bindings/arm/xen.txt > +++ b/Documentation/devicetree/bindings/arm/xen.txt > @@ -17,6 +17,19 @@ the following properties: >A GIC node is also required. >This property is unnecessary when booting Dom0 using ACPI. > > +Optional properties: > + > +- clocks: one or more clocks to be registered. > + Xen hypervisor drivers might replace native drivers, resulting in > + clocks not registered by these native drivers. To avoid that these > + unregistered clocks are disabled by the Linux kernel initialization > + register them in the hypervisor node. > + An example for this are the clocks of a serial driver already enabled > + by the firmware. If the clocks used by the serial hardware interface > + are not registered by the serial driver itself the serial output > + might stop once the Linux kernel initialization disables the 'unused' > + clocks. The above describes the set of problems, but doesn't set out the actual contract. It also covers a number of Linux implementation details in abstract. As I commented previously [1], the binding should describe the set of guarantees that you rewquire (e.g. that the clocks must be left as-is, not gated, and their rates left unchanged). Please describe the specific set of guarantees that you require. Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/440434.html ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Some clocks might be used by the Xen hypervisor and not by the Linux
kernel. If these are not registered by the Linux kernel, they might be
disabled by clk_disable_unused() as the kernel doesn't know that they
are used. The clock of the serial console handled by Xen is one
example for this. It might be disabled by clk_disable_unused() which
stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel
command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor
node. The Linux kernel has to register the clocks from the hypervisor
node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node
and if so register the given clocks to the Linux kernel clock
framework and with this mark them as used. This prevents the clocks
from being disabled.
Signed-off-by: Dirk Behme
---
Changes in v2: Drop the Linux implementation details like clk_disable_unused
in xen.txt.
Documentation/devicetree/bindings/arm/xen.txt | 13
arch/arm/xen/enlighten.c | 47 +++
2 files changed, 60 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/xen.txt
b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..21fd469 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -17,6 +17,19 @@ the following properties:
A GIC node is also required.
This property is unnecessary when booting Dom0 using ACPI.
+Optional properties:
+
+- clocks: one or more clocks to be registered.
+ Xen hypervisor drivers might replace native drivers, resulting in
+ clocks not registered by these native drivers. To avoid that these
+ unregistered clocks are disabled by the Linux kernel initialization
+ register them in the hypervisor node.
+ An example for this are the clocks of a serial driver already enabled
+ by the firmware. If the clocks used by the serial hardware interface
+ are not registered by the serial driver itself the serial output
+ might stop once the Linux kernel initialization disables the 'unused'
+ clocks.
+
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
#include
#include
+#include
#include
#include
#include
@@ -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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi, On 30/06/16 16:18, Mark Rutland wrote: On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote: On 30.06.2016 16:21, Mark Rutland wrote: On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote: +- clocks: one or more clocks to be registered. + Xen hypervisor drivers might replace native drivers, resulting in + clocks not registered by these native drivers. To avoid that these + unregistered clocks are disabled, then, e.g. by clk_disable_unused(), + register them in the hypervisor node. + An example for this are the clocks of the serial driver. If the clocks + used by the serial hardware interface are not registered by the serial + driver the serial output might stop once clk_disable_unused() is called. This shouldn't be described in terms of the Linux implementation details like clk_disable_unused. Those can change at any time, and don't define the contract as such. Ok. I remove that from the description. Thanks! Great, thanks. What exactly is the contract here? I assume that you don't want the kernel to alter the state of these clocks in any way, I don't think so. I think what we want is that the kernel just don't disable the (from kernel's point of view) "unused" clocks. I think we get this from the fact that using 'clk_ignore_unused' is a working workaround for this issue. What if the clock (or a parent thereof) is shared with another device? Surely you don't want that other driver to change the rate (changing the rate of the UART behind Xen's back)? I appreciate that clk_ignore_unused might work on platforms today, but that relies on the behaviour of drivers, which might change. It may also not work on other platforms in future, in cases like the above. e.g. the rate cannot be changed, they must be left always on, parent clocks cannot be altered if that would result in momentary jitter. I suspect it may be necessary to do more to ensure that, but I'm not familiar enough with the clocks subsystem to say. Are we likely to need to care about regulators, GPIOs, resets, etc? I worry that this may be the tip of hte iceberg I don't think so. If there is a user in the kernel of the clock, fine. Then hopefully the user in the kernel knows what he is doing with the clock and then he should do what ever he wants. I'm not sure that's true. A user of the clock may know nothing about other users. As far as I can see, nothing prevents it from changing the clock rate. While drivers in the same kernel can co-ordinate to avoid problems such as that, we can't do that if we know nothing about the user hidden by Xen. From looking at the Xen bug tracker, it's not clear which platforms/devices this is necessary for, so it's still not clear to me if we need to care about regulators, GPIOs, resets, and so on. Do we know which platforms in particular we need this for? I believe this is necessary for all the platforms where the UART has a clock described in the firmware. This workaround was first required on the arndale. Note that UART on Juno r2 has a clock described but 'clk_ignore_unused' is not necessary. I am wondering why it is working. The number of devices used by Xen is very limited (UART, timer and SMMU). However we may also want to consider device passthrough where a clock would be handled by another guest. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi, On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote: > On 30.06.2016 16:21, Mark Rutland wrote: > >On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote: > >>+- clocks: one or more clocks to be registered. > >>+ Xen hypervisor drivers might replace native drivers, resulting in > >>+ clocks not registered by these native drivers. To avoid that these > >>+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(), > >>+ register them in the hypervisor node. > >>+ An example for this are the clocks of the serial driver. If the clocks > >>+ used by the serial hardware interface are not registered by the serial > >>+ driver the serial output might stop once clk_disable_unused() is called. > > > >This shouldn't be described in terms of the Linux implementation details > >like clk_disable_unused. Those can change at any time, and don't define > >the contract as such. > > Ok. I remove that from the description. Thanks! Great, thanks. > >What exactly is the contract here? I assume that you don't want the > >kernel to alter the state of these clocks in any way, > > I don't think so. I think what we want is that the kernel just don't > disable the (from kernel's point of view) "unused" clocks. I think > we get this from the fact that using 'clk_ignore_unused' is a > working workaround for this issue. What if the clock (or a parent thereof) is shared with another device? Surely you don't want that other driver to change the rate (changing the rate of the UART behind Xen's back)? I appreciate that clk_ignore_unused might work on platforms today, but that relies on the behaviour of drivers, which might change. It may also not work on other platforms in future, in cases like the above. > >e.g. the rate cannot be changed, they must be left always on, parent > >clocks cannot be altered if that would result in momentary jitter. > > > >I suspect it may be necessary to do more to ensure that, but I'm not > >familiar enough with the clocks subsystem to say. > > > >Are we likely to need to care about regulators, GPIOs, resets, etc? I > >worry that this may be the tip of hte iceberg > > I don't think so. If there is a user in the kernel of the clock, > fine. Then hopefully the user in the kernel knows what he is doing > with the clock and then he should do what ever he wants. I'm not sure that's true. A user of the clock may know nothing about other users. As far as I can see, nothing prevents it from changing the clock rate. While drivers in the same kernel can co-ordinate to avoid problems such as that, we can't do that if we know nothing about the user hidden by Xen. From looking at the Xen bug tracker, it's not clear which platforms/devices this is necessary for, so it's still not clear to me if we need to care about regulators, GPIOs, resets, and so on. Do we know which platforms in particular we need this for? > As there is a user in the kernel, we don't have to care about > 'accidentally' disabling it. > > Just let us care about the clocks there is no user. As above, I don't believe that alone is sufficient in general, though it may happen to be for a particular platform. Without information regarding the affected platform(s), it's rather difficult to tell. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 30.06.2016 16:21, Mark Rutland wrote:
On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote:
Some clocks might be used by the Xen hypervisor and not by the Linux
kernel. If these are not registered by the Linux kernel, they might be
disabled by clk_disable_unused() as the kernel doesn't know that they
are used. The clock of the serial console handled by Xen is one
example for this. It might be disabled by clk_disable_unused() which
stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel
command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor
node. The Linux kernel has to register the clocks from the hypervisor
node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node
and if so register the given clocks to the Linux kernel clock
framework and with this mark them as used. This prevents the clocks
from being disabled.
Signed-off-by: Dirk Behme
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
Documentation/devicetree/bindings/arm/xen.txt | 11 +++
arch/arm/xen/enlighten.c | 47 +++
2 files changed, 58 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/xen.txt
b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..55dfd3b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -17,6 +17,17 @@ the following properties:
A GIC node is also required.
This property is unnecessary when booting Dom0 using ACPI.
+Optional properties:
+
+- clocks: one or more clocks to be registered.
+ Xen hypervisor drivers might replace native drivers, resulting in
+ clocks not registered by these native drivers. To avoid that these
+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+ register them in the hypervisor node.
+ An example for this are the clocks of the serial driver. If the clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.
This shouldn't be described in terms of the Linux implementation details
like clk_disable_unused. Those can change at any time, and don't define
the contract as such.
Ok. I remove that from the description. Thanks!
What exactly is the contract here? I assume that you don't want the
kernel to alter the state of these clocks in any way,
I don't think so. I think what we want is that the kernel just don't
disable the (from kernel's point of view) "unused" clocks. I think we
get this from the fact that using 'clk_ignore_unused' is a working
workaround for this issue.
e.g. the rate
cannot be changed, they must be left always on, parent clocks cannot be
altered if that would result in momentary jitter.
I suspect it may be necessary to do more to ensure that, but I'm not
familiar enough with the clocks subsystem to say.
>
Are we likely to need to care about regulators, GPIOs, resets, etc? I
worry that this may be the tip of hte iceberg
I don't think so. If there is a user in the kernel of the clock, fine.
Then hopefully the user in the kernel knows what he is doing with the
clock and then he should do what ever he wants. As there is a user in
the kernel, we don't have to care about 'accidentally' disabling it.
Just let us care about the clocks there is no user.
Best regards
Dirk
+
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
#include
#include
+#include
#include
#include
#include
@@ -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
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote:
> Some clocks might be used by the Xen hypervisor and not by the Linux
> kernel. If these are not registered by the Linux kernel, they might be
> disabled by clk_disable_unused() as the kernel doesn't know that they
> are used. The clock of the serial console handled by Xen is one
> example for this. It might be disabled by clk_disable_unused() which
> stops the whole serial output, even from Xen, then.
>
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
>
> http://bugs.xenproject.org/xen/bug/45
>
> too.
>
> To fix this, we will add the "unused" clocks in Xen to the hypervisor
> node. The Linux kernel has to register the clocks from the hypervisor
> node, then.
>
> Therefore, check if there is a "clocks" entry in the hypervisor node
> and if so register the given clocks to the Linux kernel clock
> framework and with this mark them as used. This prevents the clocks
> from being disabled.
>
> Signed-off-by: Dirk Behme
> ---
> Changes in v2:
> - Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
> for-linus-4.8
> - Add changes to Documentation/devicetree/bindings/arm/xen.txt
>
> Documentation/devicetree/bindings/arm/xen.txt | 11 +++
> arch/arm/xen/enlighten.c | 47
> +++
> 2 files changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> b/Documentation/devicetree/bindings/arm/xen.txt
> index c9b9321..55dfd3b 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -17,6 +17,17 @@ the following properties:
>A GIC node is also required.
>This property is unnecessary when booting Dom0 using ACPI.
>
> +Optional properties:
> +
> +- clocks: one or more clocks to be registered.
> + Xen hypervisor drivers might replace native drivers, resulting in
> + clocks not registered by these native drivers. To avoid that these
> + unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
> + register them in the hypervisor node.
> + An example for this are the clocks of the serial driver. If the clocks
> + used by the serial hardware interface are not registered by the serial
> + driver the serial output might stop once clk_disable_unused() is called.
This shouldn't be described in terms of the Linux implementation details
like clk_disable_unused. Those can change at any time, and don't define
the contract as such.
What exactly is the contract here? I assume that you don't want the
kernel to alter the state of these clocks in any way, e.g. the rate
cannot be changed, they must be left always on, parent clocks cannot be
altered if that would result in momentary jitter.
I suspect it may be necessary to do more to ensure that, but I'm not
familiar enough with the clocks subsystem to say.
Are we likely to need to care about regulators, GPIOs, resets, etc? I
worry that this may be the tip of hte iceberg, and we might need a
better way of catering for the general case of resources Xen wants to
own.
Thanks,
Mark.
> +
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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;
> + }
> + }
> +
> + r
[Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Some clocks might be used by the Xen hypervisor and not by the Linux
kernel. If these are not registered by the Linux kernel, they might be
disabled by clk_disable_unused() as the kernel doesn't know that they
are used. The clock of the serial console handled by Xen is one
example for this. It might be disabled by clk_disable_unused() which
stops the whole serial output, even from Xen, then.
Up to now, the workaround for this has been to use the Linux kernel
command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor
node. The Linux kernel has to register the clocks from the hypervisor
node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node
and if so register the given clocks to the Linux kernel clock
framework and with this mark them as used. This prevents the clocks
from being disabled.
Signed-off-by: Dirk Behme
---
Changes in v2:
- Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git
for-linus-4.8
- Add changes to Documentation/devicetree/bindings/arm/xen.txt
Documentation/devicetree/bindings/arm/xen.txt | 11 +++
arch/arm/xen/enlighten.c | 47 +++
2 files changed, 58 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/xen.txt
b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..55dfd3b 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -17,6 +17,17 @@ the following properties:
A GIC node is also required.
This property is unnecessary when booting Dom0 using ACPI.
+Optional properties:
+
+- clocks: one or more clocks to be registered.
+ Xen hypervisor drivers might replace native drivers, resulting in
+ clocks not registered by these native drivers. To avoid that these
+ unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
+ register them in the hypervisor node.
+ An example for this are the clocks of the serial driver. If the clocks
+ used by the serial hardware interface are not registered by the serial
+ driver the serial output might stop once clk_disable_unused() is called.
+
To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
under /hypervisor with following parameters:
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
#include
#include
+#include
#include
#include
#include
@@ -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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
