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

[Michael Turquette]

Quoting Julien Grall (2016-07-20 06:21:45)
> 
> 
> On 20/07/2016 13:46, Geert Uytterhoeven wrote:
> > Hi Julien,
> 
> Hello Geert,
> 
> > On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall  wrote:
> >> On 20/07/16 12:49, Geert Uytterhoeven wrote:
> >>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall 
> >>> wrote:
>  On 20/07/16 10:43, Geert Uytterhoeven wrote:
> > On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme 
> > wrote:
> >> Clocks described by this property are reserved for use by Xen, and the
> >> OS
> >> must not alter their state any way, such as disabling or gating a
> >> clock,
> >> or modifying its rate. Ensuring this may impose constraints on parent
> >> clocks or other resources used by the clock tree.
> >>
> >> This property is used to proxy clocks for devices Xen has taken
> >> ownership
> >> of, such as UARTs, for which the associated clock controller(s) remain
> >> under the control of Dom0.
> >
> > I'm not familiar with using XEN at all, but I'm a bit puzzled...
> >
> > Can't you just add a clocks property to the (virtual) serial device node
> > in DT?
> > Then the (virtual) serial device driver can get and enable the clock?
> 
>  There is no DT node for the Xen console (hvc). The UART used by Xen will
>  be
>  completely removed from the Device tree.
> >>>
> >>> Why is it removed?
> >>
> >> Because the device is used exclusively by Xen and DOM0 should not touch it
> >> at all (IRQs and MMIOs are not mapped).
> >
> > IMHO then it's Xen's responsability to make sure not to disable the 
> > clock(s).
> 
> Xen does not disable the clock(s). The problem is Linux thinks the clock 
> is not used and therefore will disable the clock.
> 
> Actually Xen does not have any knowledge how to handle clocks (i.e 
> setting rate, enabling/disabling). I would recommend you to read the 
> binding description in the patch and not the implementation which is 
> IHMO misleading.
> 
> >
> > Who removes the device node from the DT? If Xen, can't it just remember
> > which clocks were present in removed device nodes?
> 
> Xen is removing the node from the DT. Not removing the node will not 
> change the problem. Linux is disabling the clock because there is no 
> driver which used those clocks.
> 
> As the clock subsystem will disable any unused clocks (from Linux point 
> of view), the UART will get disabled and the console will be lost.
> 
> What we want to achieve with this patch is to let Linux knows that this 
> clock is in use by someone else (hypervisor or guest) and:
>  * If the clock is not shared: don't disable it
>  * If the clock is shared: don't change the rate

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

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

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

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

Regards,
Mike

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

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

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

[Michael Turquette]

Quoting Stefano Stabellini (2016-07-14 03:38:04)
> On Thu, 14 Jul 2016, Dirk Behme wrote:
> > On 13.07.2016 23:03, Michael Turquette wrote:
> > > Quoting Dirk Behme (2016-07-13 11:56:30)
> > > > On 13.07.2016 20:43, Stefano Stabellini wrote:
> > > > > On Wed, 13 Jul 2016, Dirk Behme wrote:
> > > > > > On 13.07.2016 00:26, Michael Turquette wrote:
> > > > > > > Quoting Dirk Behme (2016-07-12 00:46:45)
> > > > > > > > Clocks described by this property are reserved for use by Xen, 
> > > > > > > > and
> > > > > > > > the OS
> > > > > > > > must not alter their state any way, such as disabling or gating 
> > > > > > > > a
> > > > > > > > clock,
> > > > > > > > or modifying its rate. Ensuring this may impose constraints on
> > > > > > > > parent
> > > > > > > > clocks or other resources used by the clock tree.
> > > > > > > 
> > > > > > > Note that clk_prepare_enable will not prevent the rate from 
> > > > > > > changing
> > > > > > > (clk_set_rate) or a parent from changing (clk_set_parent). The 
> > > > > > > only
> > > > > > > way
> > > > > > > to do this currently would be to set the following flags on the
> > > > > > > effected
> > > > > > > clocks:
> > > > > > > 
> > > > > > > CLK_SET_RATE_GATE
> > > > > > > CLK_SET_PARENT_GATE
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Regarding setting flags, I think we already talked about that. I 
> > > > > > think
> > > > > > the
> > > > > > conclusion was that in our case its not possible to manipulate the
> > > > > > flags in
> > > > > > the OS as this isn't intended to be done in cases like ours. 
> > > > > > Therefore
> > > > > > no API
> > > > > > is exported for this.
> > > > > > 
> > > > > > I.e. if we need to set these flags, we have to do that in Xen where 
> > > > > > we
> > > > > > add the
> > > > > > clocks to the hypervisor node in the device tree. And not in the
> > > > > > kernel patch
> > > > > > discussed here.
> > > > > 
> > > > > These are internal Linux flags, aren't they?
> > > > 
> > > > 
> > > > I've been under the impression that you can set clock "flags" via the
> > > > device tree. Seems I need to re-check that ;)
> > > 
> > > Right, you cannot set flags from the device tree. Also, setting these
> > > flags is done by the clock provider driver, not a consumer. Xen is the
> > > consumer.
> > 
> > 
> > Ok, thanks, then I think we can forget about using flags for the issue we 
> > are
> > discussing here.
> > 
> > Best regards
> > 
> > Dirk
> > 
> > P.S.: Would it be an option to merge the v4 patch we are discussing here,
> > then? From the discussion until here, it sounds to me that it's the best
> > option we have at the moment. Maybe improving it in the future, then.
> 
> It might be a step in the right direction, but it doesn't really prevent
> clk_set_rate from changing properties of a clock owned by Xen.  This
> patch is incomplete. We need to understand at least what it would take
> to have a complete solution.
> 
> Michael, do you have any suggestions on how it would be possible to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
> way?

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

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

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

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

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

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

Regards,
Mike

> 

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

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

[Michael Turquette]

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

Right, you cannot set flags from the device tree. Also, setting these
flags is done by the clock provider driver, not a consumer. Xen is the
consumer.

Regards,
Mike

> 
> Best regards
> 
> Dirk
> 
> 
> > They cannot be set in Xen.
> >
> > If the only way to make sure that clk_set_rate and clk_set_parent fail
> > on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and
> > CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing
> > a new internal Linux API.
> >
> >
> >>> And then enable the clocks. All calls to clk_set_parent and clk_set_rate
> >>> with those clocks in the path will fail, so long as they are prepared
> >>> and enabled. This implementation detail is specific to Linux and
> >>> definitely should not go into the binding.
> >>>
> >>>>
> >>>> This property is used to proxy clocks for devices Xen has taken ownership
> >>>> of, such as UARTs, for which the associated clock controller(s) remain
> >>>> under the control of Dom0.
> >>>
> >>> I'm still not completely sure about this type of layering and whether or
> >>> not it is sane. If you want Xen to manage the clock controller, AND you
> >>> want Linux guests or dom0 to consume those clocks and manipulate them in
> >>> other drivers, then this solution won't work.
> >>>
> >>> Regards,
> >>> Mike
> >>>
> >>>>
> >>>> Up to now, the workaround for this has been to use the Linux kernel
> >>>> command line parameter 'clk_ignore_unused'. See Xen bug
> >>>>
> >>>> http://bugs.xenproject.org/xen/bug/45
> >>>>
> >>>> too.
> >>>>
> >>>> Signed-off-by: Dirk Behme <dirk.be...@de.bosch.com>
> >>>> ---
> >>>> Changes in v4: Switch to the xen.txt description proposed by Mark:
> >>>> https://www.spinics.net/lists/arm-kernel/msg516158.html
> >>>>
> >>>> Changes in v3: Use the xen.txt description proposed by Michael. Thanks!
> >>>>
> >>>> Changes in v2: Drop the Linux implementation details like
> >>>> clk_disable_unused
> >>>> in xen.txt.
> >>>>
> >>>>   Documentation/devicetree/bindings/arm/xen.txt | 12 +++
> >>>>   arch/arm/xen/enlighten.c  | 47
> >>>> +++
> >>>>   2 files changed, 59 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/arm/xen.txt
> >>>> b/Documentation/devicetree/bindings/arm/xen.txt
> >>>> index c9b9321..437e50b 100644
> >>>> --- a/Documentation/devicetree/binding

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

[Michael Turquette]

Quoting Dirk Behme (2016-07-12 00:46:45)
> Clocks described by this property are reserved for use by Xen, and the OS
> must not alter their state any way, such as disabling or gating a clock,
> or modifying its rate. Ensuring this may impose constraints on parent
> clocks or other resources used by the clock tree.

Note that clk_prepare_enable will not prevent the rate from changing
(clk_set_rate) or a parent from changing (clk_set_parent). The only way
to do this currently would be to set the following flags on the effected
clocks:

CLK_SET_RATE_GATE
CLK_SET_PARENT_GATE

And then enable the clocks. All calls to clk_set_parent and clk_set_rate
with those clocks in the path will fail, so long as they are prepared
and enabled. This implementation detail is specific to Linux and
definitely should not go into the binding.

> 
> This property is used to proxy clocks for devices Xen has taken ownership
> of, such as UARTs, for which the associated clock controller(s) remain
> under the control of Dom0.

I'm still not completely sure about this type of layering and whether or
not it is sane. If you want Xen to manage the clock controller, AND you
want Linux guests or dom0 to consume those clocks and manipulate them in
other drivers, then this solution won't work.

Regards,
Mike

> 
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
> 
> http://bugs.xenproject.org/xen/bug/45
> 
> too.
> 
> Signed-off-by: Dirk Behme 
> ---
> 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 
>  #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;
> +}
> 

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

[Michael Turquette]

Quoting Julien Grall (2016-07-12 02:21:12)
> Hi Mike,
> 
> On 08/07/16 18:06, Michael Turquette wrote:
> > Quoting Julien Grall (2016-07-08 02:34:43)
> >> Hi Dirk,
> >>
> >> On 08/07/16 08:44, Dirk Behme wrote:
> >>> 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
> >>
> >> I would say "An example is the UART used by the hypervisor."
> >>
> >>> consume and enable these clocks in the OS to ensure behavior continues
> >>> after firmware configures the UART hardware and corresponding clock
> >>> harder.
> >>
> >> What do you mean by "harder"?
> >>
> >> Also, relying on DOM0 to enable the clock looks very wrong to me and you
> >> give an example which prove that. The UART will be used before hand by
> >> Xen, however it will not be possible to use it if you expect DOM0 to
> >> enable the clock (or even modify the clock frequency).
> >>
> >> The clock should be enabled either by the firmware or Xen. But not DOM0.
> >> DOM0 should not touch this clock at all.
> >>
> >> Furthermore, this property could be used for clock associated to device
> >> that will be passthrough-ed to a guest. In this case, the clock would be
> >> enabled even if the device is not in use which will result more power
> >> consumption.
> >
> > Is there a need to pass clock references through to guests? If so the
> > unmerged CLK_ENABLE_HAND_OFF[0] feature might be useful to you? If this
> > flag is set on a given clk, it will be enabled at the time it is
> > registered by the clk provider driver, and it's enable_count reference
> > counter will be "handed off" to the first consumer that calls clk_get()
> > and clk_prepare_enable() on it. This means the clock CAN be gated by
> > it's proper driver, but it will be enabled at boot time as well.
> 
> Some driver requires to have the clock in hand to be able to configure 
> the clock (such as setting the rate). So we would have to find a way to 
> let the guest using the clock either by assigning the clock or some PV 
> clock driver.
> 
> However, platform device passthrough (i.e non-pci device) cannot be done 
> generically. The user has to provide a lots of information manually 
> (such as MMIO, IRQ, device tree node...). So I am not sure if we want to 
> have a generic solution here.
> 
> I though it would be worth to mention it because we may (or not) use 
> this clock to tell DOM0 (don't touch it).
> 
> > This is useful for use cases like splash screens where the bootloader
> > configures the display and plays some animation, and we want the linux
> > kernel to take over the display controller hardware without cutting
> > clocks, blanking or reseting it. Handing off the clock reference count
> > helps achieve this.
> 
>  From my understanding, any device used by Xen would be in a similar 
> situation, although there will be no driver in Linux. The current patch 
> (as well as the v4) is calling clk_prepare_enable for each clock used by 
> Xen. Could enabling the clock create unexpected behavior such as the 
> UART loosing/dropping characters?

In general, no, it will not cause any bad side effect.

Regards,
Mike

> 
> Regards,
> 
> -- 
> Julien Grall

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

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

[Michael Turquette]

Quoting Julien Grall (2016-07-08 02:34:43)
> Hi Dirk,
> 
> On 08/07/16 08:44, Dirk Behme wrote:
> > 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
> 
> I would say "An example is the UART used by the hypervisor."
> 
> > consume and enable these clocks in the OS to ensure behavior continues
> > after firmware configures the UART hardware and corresponding clock
> > harder.
> 
> What do you mean by "harder"?
> 
> Also, relying on DOM0 to enable the clock looks very wrong to me and you 
> give an example which prove that. The UART will be used before hand by 
> Xen, however it will not be possible to use it if you expect DOM0 to 
> enable the clock (or even modify the clock frequency).
> 
> The clock should be enabled either by the firmware or Xen. But not DOM0. 
> DOM0 should not touch this clock at all.
> 
> Furthermore, this property could be used for clock associated to device 
> that will be passthrough-ed to a guest. In this case, the clock would be 
> enabled even if the device is not in use which will result more power 
> consumption.

Is there a need to pass clock references through to guests? If so the
unmerged CLK_ENABLE_HAND_OFF[0] feature might be useful to you? If this
flag is set on a given clk, it will be enabled at the time it is
registered by the clk provider driver, and it's enable_count reference
counter will be "handed off" to the first consumer that calls clk_get()
and clk_prepare_enable() on it. This means the clock CAN be gated by
it's proper driver, but it will be enabled at boot time as well.

This is useful for use cases like splash screens where the bootloader
configures the display and plays some animation, and we want the linux
kernel to take over the display controller hardware without cutting
clocks, blanking or reseting it. Handing off the clock reference count
helps achieve this.

[0] lkml.kernel.org/g/1455225554-13267-1-git-send-email-mturque...@baylibre.com

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.
> >
> > To fix this, we will add the "unused" clocks in Xen to the hypervisor
> > node. The OS has to consume and enable the clocks from the hypervisor
> > node, then.
> >
> > Therefore, check if there is a "clocks" entry in the hypervisor node
> > and if so consume and enable the given clocks. This prevents the clocks
> > from being disabled by the OS.
> >
> > Signed-off-by: Dirk Behme 
> > ---
> > 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 | 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..00f2165 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 enabled
> > +  Xen hypervisor drivers might replace native OS drivers. The result is
> 
> "native OS" has no meaning on Xen. This seems to be a cumbersome way to 
> say that the device will be used by the hypervisor and hidden to DOM0 
> (aka hardware domain).
> 
> > +  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.
> > +
> >   To support UEFI on Xen ARM virtual platforms, Xen populates the FDT 
> > "uefi" node
> >   under /hypervisor with following parameters:
> 
> Regards,
> 
> -- 
> Julien Grall

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

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

[Michael Turquette]

Quoting Julien Grall (2016-07-08 03:49:33)
> 
> 
> On 08/07/16 11:40, Dirk Behme wrote:
> > Hi Michael and Julien,
> >
> > On 08.07.2016 11:34, Julien Grall wrote:
> >> Hi Dirk,
> >>
> >> On 08/07/16 08:44, Dirk Behme wrote:
> >>> 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
> >>
> >> I would say "An example is the UART used by the hypervisor."
> >>
> >>> consume and enable these clocks in the OS to ensure behavior continues
> >>> after firmware configures the UART hardware and corresponding clock
> >>> harder.
> >>
> >> What do you mean by "harder"?
> >>
> >> Also, relying on DOM0 to enable the clock looks very wrong to me and you
> >> give an example which prove that. The UART will be used before hand by
> >> Xen, however it will not be possible to use it if you expect DOM0 to
> >> enable the clock (or even modify the clock frequency).
> >>
> >> The clock should be enabled either by the firmware or Xen. But not DOM0.
> >> DOM0 should not touch this clock at all.
> >>
> >> Furthermore, this property could be used for clock associated to device
> >> that will be passthrough-ed to a guest. In this case, the clock would be
> >> enabled even if the device is not in use which will result more power
> >> consumption.
> >
> >
> > I took the description directly from Michael's proposal
> >
> > http://www.spinics.net/lists/arm-kernel/msg516576.html
> >
> > Would it be possible that you two experts agree on the exact wording you
> > like to see?
> 
> I think the wording suggested by Mark in [1] represents better what we 
> would like to achieve with this property.

Mark's copy is fine by me. The main thing is to avoid talking about
"unused" clocks being disabled by the OS. If you have need clocks to be
enabled, then you simply claim them and enable them. All of the other
stuff is just noise.

Regards,
Mike

> 
> Regards,
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg516158.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

[Michael Turquette]

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 t

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

[Michael Turquette]

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

[Michael Turquette]

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

[Michael Turquette]

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)) {
> +