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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 13 Jul 2016, Dirk Behme wrote:
> On 13.07.2016 00:26, Michael Turquette wrote:
> > Quoting Dirk Behme (2016-07-12 00:46:45)
> > > Clocks described by this property are reserved for use by Xen, and the OS
> > > must not alter their state any way, such as disabling or gating a clock,
> > > or modifying its rate. Ensuring this may impose constraints on parent
> > > clocks or other resources used by the clock tree.
> > 
> > Note that clk_prepare_enable will not prevent the rate from changing
> > (clk_set_rate) or a parent from changing (clk_set_parent). The only way
> > to do this currently would be to set the following flags on the effected
> > clocks:
> > 
> >     CLK_SET_RATE_GATE
> >     CLK_SET_PARENT_GATE
> 
> 
> 
> Regarding setting flags, I think we already talked about that. I think the
> conclusion was that in our case its not possible to manipulate the flags in
> the OS as this isn't intended to be done in cases like ours. Therefore no API
> is exported for this.
> 
> I.e. if we need to set these flags, we have to do that in Xen where we add the
> clocks to the hypervisor node in the device tree. And not in the kernel patch
> discussed here.

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

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


> > And then enable the clocks. All calls to clk_set_parent and clk_set_rate
> > with those clocks in the path will fail, so long as they are prepared
> > and enabled. This implementation detail is specific to Linux and
> > definitely should not go into the binding.
> > 
> > > 
> > > This property is used to proxy clocks for devices Xen has taken ownership
> > > of, such as UARTs, for which the associated clock controller(s) remain
> > > under the control of Dom0.
> > 
> > I'm still not completely sure about this type of layering and whether or
> > not it is sane. If you want Xen to manage the clock controller, AND you
> > want Linux guests or dom0 to consume those clocks and manipulate them in
> > other drivers, then this solution won't work.
> > 
> > Regards,
> > Mike
> > 
> > > 
> > > Up to now, the workaround for this has been to use the Linux kernel
> > > command line parameter 'clk_ignore_unused'. See Xen bug
> > > 
> > > http://bugs.xenproject.org/xen/bug/45
> > > 
> > > too.
> > > 
> > > Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
> > > ---
> > > Changes in v4: Switch to the xen.txt description proposed by Mark:
> > >                https://www.spinics.net/lists/arm-kernel/msg516158.html
> > > 
> > > Changes in v3: Use the xen.txt description proposed by Michael. Thanks!
> > > 
> > > Changes in v2: Drop the Linux implementation details like
> > > clk_disable_unused
> > >                in xen.txt.
> > > 
> > >  Documentation/devicetree/bindings/arm/xen.txt | 12 +++++++
> > >  arch/arm/xen/enlighten.c                      | 47
> > > +++++++++++++++++++++++++++
> > >  2 files changed, 59 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> > > b/Documentation/devicetree/bindings/arm/xen.txt
> > > index c9b9321..437e50b 100644
> > > --- a/Documentation/devicetree/bindings/arm/xen.txt
> > > +++ b/Documentation/devicetree/bindings/arm/xen.txt
> > > @@ -17,6 +17,18 @@ the following properties:
> > >    A GIC node is also required.
> > >    This property is unnecessary when booting Dom0 using ACPI.
> > > 
> > > +Optional properties:
> > > +
> > > +- clocks: a list of phandle + clock-specifier pairs
> > > +  Clocks described by this property are reserved for use by Xen, and the
> > > +  OS must not alter their state any way, such as disabling or gating a
> > > +  clock, or modifying its rate. Ensuring this may impose constraints on
> > > +  parent clocks or other resources used by the clock tree.
> > > +
> > > +  Note: this property is used to proxy clocks for devices Xen has taken
> > > +  ownership of, such as UARTs, for which the associated clock
> > > +  controller(s) remain under the control of Dom0.
> > > +
> > >  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT
> > > "uefi" node
> > >  under /hypervisor with following parameters:
> > > 
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index 47acb36..5c546d0 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/of_fdt.h>
> > >  #include <linux/of_irq.h>
> > >  #include <linux/of_address.h>
> > > +#include <linux/clk-provider.h>
> > >  #include <linux/cpuidle.h>
> > >  #include <linux/cpufreq.h>
> > >  #include <linux/cpu.h>
> > > @@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
> > >  }
> > >  late_initcall(xen_pm_init);
> > > 
> > > +/*
> > > + * Check if we want to register some clocks, that they
> > > + * are not freed because unused by clk_disable_unused().
> > > + * E.g. the serial console clock.
> > > + */
> > > +static int __init xen_arm_register_clks(void)
> > > +{
> > > +       struct clk *clk;
> > > +       struct device_node *xen_node;
> > > +       unsigned int i, count;
> > > +       int ret = 0;
> > > +
> > > +       xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> > > +       if (!xen_node) {
> > > +               pr_err("Xen support was detected before, but it has
> > > disappeared\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       count = of_clk_get_parent_count(xen_node);
> > > +       if (!count)
> > > +               goto out;
> > > +
> > > +       for (i = 0; i < count; i++) {
> > > +               clk = of_clk_get(xen_node, i);
> > > +               if (IS_ERR(clk)) {
> > > +                       pr_err("Xen failed to register clock %i. Error:
> > > %li\n",
> > > +                              i, PTR_ERR(clk));
> > > +                       ret = PTR_ERR(clk);
> > > +                       goto out;
> > > +               }
> > > +
> > > +               ret = clk_prepare_enable(clk);
> > > +               if (ret < 0) {
> > > +                       pr_err("Xen failed to enable clock %i. Error:
> > > %i\n",
> > > +                              i, ret);
> > > +                       goto out;
> > > +               }
> > > +       }
> > > +
> > > +       ret = 0;
> > > +
> > > +out:
> > > +       of_node_put(xen_node);
> > > +       return ret;
> > > +}
> > > +late_initcall(xen_arm_register_clks);
> > > 
> > >  /* empty stubs */
> > >  void xen_arch_pre_suspend(void) { }
> > > --
> 

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