Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Dirk Behme <dirk.behme@xxxxxxxxxxxx>]

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 <dirk.behme@xxxxxxxxxxxx>
> > ---
> > 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 <linux/of_fdt.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_address.h>
> > +#include <linux/clk-provider.h>
> s/clk-provider.h/clk.h/
>
> clk-provider.h is only used for providers and this bit of code is a
> consumer.

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


> >  #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);
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel