| Message ID | 1467701423-31138-1-git-send-email-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dirk.behme@de.bosch.com> > --- > 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
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 <dirk.behme@de.bosch.com> >> --- >> 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
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.
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.
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 <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) { }
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 <dirk.behme@de.bosch.com> --- 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(+)