* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-13 21:03 ` Michael Turquette
0 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-13 21:03 UTC (permalink / raw)
To: linux-arm-kernel
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.behme@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/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 at lists.xen.org
> > https://lists.xen.org/xen-devel
> >
>
^ permalink raw reply [flat|nested] 120+ messages in thread* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-13 21:03 ` Michael Turquette
0 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-13 21:03 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini
Cc: Dirk Behme, Mark Rutland, devicetree, Stephen Boyd, Julien Grall,
xen-devel, linux-clk, linux-arm-kernel
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 t=
he OS
> >>>> must not alter their state any way, such as disabling or gating a cl=
ock,
> >>>> 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 w=
ay
> >>> to do this currently would be to set the following flags on the effec=
ted
> >>> 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 fla=
gs 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 kerne=
l 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_r=
ate
> >>> 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 owne=
rship
> >>>> of, such as UARTs, for which the associated clock controller(s) rema=
in
> >>>> 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 y=
ou
> >>> 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@de.bosch.com>
> >>>> ---
> >>>> Changes in v4: Switch to the xen.txt description proposed by Mark:
> >>>> https://www.spinics.net/lists/arm-kernel/msg516158.h=
tml
> >>>>
> >>>> Changes in v3: Use the xen.txt description proposed by Michael. Than=
ks!
> >>>>
> >>>> 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, an=
d the
> >>>> + OS must not alter their state any way, such as disabling or gatin=
g a
> >>>> + clock, or modifying its rate. Ensuring this may impose constraint=
s on
> >>>> + parent clocks or other resources used by the clock tree.
> >>>> +
> >>>> + Note: this property is used to proxy clocks for devices Xen has t=
aken
> >>>> + 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 =3D 0;
> >>>> +
> >>>> + xen_node =3D 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 =3D of_clk_get_parent_count(xen_node);
> >>>> + if (!count)
> >>>> + goto out;
> >>>> +
> >>>> + for (i =3D 0; i < count; i++) {
> >>>> + clk =3D of_clk_get(xen_node, i);
> >>>> + if (IS_ERR(clk)) {
> >>>> + pr_err("Xen failed to register clock %i. Err=
or:
> >>>> %li\n",
> >>>> + i, PTR_ERR(clk));
> >>>> + ret =3D PTR_ERR(clk);
> >>>> + goto out;
> >>>> + }
> >>>> +
> >>>> + ret =3D clk_prepare_enable(clk);
> >>>> + if (ret < 0) {
> >>>> + pr_err("Xen failed to enable clock %i. Error:
> >>>> %i\n",
> >>>> + i, ret);
> >>>> + goto out;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + ret =3D 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@lists.xen.org
> > https://lists.xen.org/xen-devel
> >
>=20
^ permalink raw reply [flat|nested] 120+ messages in thread* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-13 21:03 ` Michael Turquette
(?)
(?)
@ 2016-07-14 6:31 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 6:31 UTC (permalink / raw)
To: Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, Julien Grall, xen-devel,
linux-clk, linux-arm-kernel
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.
>>> 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@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/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@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-13 21:03 ` Michael Turquette
(?)
@ 2016-07-14 6:31 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 6:31 UTC (permalink / raw)
To: Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, Julien Grall, xen-devel,
linux-clk, linux-arm-kernel
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.
>>> 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@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/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) { }
^ permalink raw reply [flat|nested] 120+ messages in thread* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 6:31 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 6:31 UTC (permalink / raw)
To: linux-arm-kernel
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.
>>> 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@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/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) { }
^ permalink raw reply [flat|nested] 120+ messages in thread* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 6:31 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 6:31 UTC (permalink / raw)
To: Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, Julien Grall, xen-devel,
linux-clk, linux-arm-kernel
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.
>>> 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@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/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) { }
^ permalink raw reply [flat|nested] 120+ messages in thread* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 6:31 ` Dirk Behme
(?)
(?)
@ 2016-07-14 10:14 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 10:14 UTC (permalink / raw)
To: Dirk Behme, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, xen-devel, linux-clk,
linux-arm-kernel
Hi Dirk,
On 14/07/16 07:31, 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.
I think it is a good start, although I would like to see some rationale
in the code and commit message about the behavior of the Linux with
those clocks after this patch because it does not match the "contract".
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 6:31 ` Dirk Behme
` (2 preceding siblings ...)
(?)
@ 2016-07-14 10:38 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:38 UTC (permalink / raw)
To: Dirk Behme
Cc: Mark Rutland, devicetree, Stefano Stabellini, Michael Turquette,
Stephen Boyd, Dirk Behme, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
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?
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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread[parent not found: <7df784ab-d0c0-939b-393e-214535c4b191-V5te9oGctAVWk0Htik3J/w@public.gmane.org>]
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 6:31 ` Dirk Behme
(?)
@ 2016-07-14 10:14 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 10:14 UTC (permalink / raw)
To: Dirk Behme, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd,
xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hi Dirk,
On 14/07/16 07:31, 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.
I think it is a good start, although I would like to see some rationale
in the code and commit message about the behavior of the Linux with
those clocks after this patch because it does not match the "contract".
Regards,
--
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:14 ` Julien Grall
0 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 10:14 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dirk,
On 14/07/16 07:31, 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.
I think it is a good start, although I would like to see some rationale
in the code and commit message about the behavior of the Linux with
those clocks after this patch because it does not match the "contract".
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:14 ` Julien Grall
0 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 10:14 UTC (permalink / raw)
To: Dirk Behme, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, xen-devel, linux-clk,
linux-arm-kernel
Hi Dirk,
On 14/07/16 07:31, 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.
I think it is a good start, although I would like to see some rationale
in the code and commit message about the behavior of the Linux with
those clocks after this patch because it does not match the "contract".
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:14 ` Julien Grall
(?)
@ 2016-07-14 10:32 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:32 UTC (permalink / raw)
To: Julien Grall, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 12:14, Julien Grall wrote:
> Hi Dirk,
>
> On 14/07/16 07:31, 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.
>
> I think it is a good start, although I would like to see some rationale
> in the code and commit message about the behavior of the Linux with
> those clocks after this patch because it does not match the "contract".
I'd be happy about any wording proposals :)
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:32 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:32 UTC (permalink / raw)
To: linux-arm-kernel
On 14.07.2016 12:14, Julien Grall wrote:
> Hi Dirk,
>
> On 14/07/16 07:31, 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.
>
> I think it is a good start, although I would like to see some rationale
> in the code and commit message about the behavior of the Linux with
> those clocks after this patch because it does not match the "contract".
I'd be happy about any wording proposals :)
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:32 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:32 UTC (permalink / raw)
To: Julien Grall, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 12:14, Julien Grall wrote:
> Hi Dirk,
>
> On 14/07/16 07:31, 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.
>
> I think it is a good start, although I would like to see some rationale
> in the code and commit message about the behavior of the Linux with
> those clocks after this patch because it does not match the "contract".
I'd be happy about any wording proposals :)
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:14 ` Julien Grall
` (2 preceding siblings ...)
(?)
@ 2016-07-14 10:32 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:32 UTC (permalink / raw)
To: Julien Grall, Michael Turquette, Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Stephen Boyd, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 12:14, Julien Grall wrote:
> Hi Dirk,
>
> On 14/07/16 07:31, 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.
>
> I think it is a good start, although I would like to see some rationale
> in the code and commit message about the behavior of the Linux with
> those clocks after this patch because it does not match the "contract".
I'd be happy about any wording proposals :)
Best regards
Dirk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 6:31 ` Dirk Behme
(?)
@ 2016-07-14 10:38 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:38 UTC (permalink / raw)
To: Dirk Behme
Cc: Michael Turquette, Dirk Behme, Stefano Stabellini, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd, Julien Grall,
xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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?
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?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:38 ` Stefano Stabellini
0 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:38 UTC (permalink / raw)
To: linux-arm-kernel
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?
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?
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:38 ` Stefano Stabellini
0 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:38 UTC (permalink / raw)
To: Dirk Behme
Cc: Michael Turquette, Dirk Behme, Stefano Stabellini, Mark Rutland,
devicetree, Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
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?
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?
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:38 ` Stefano Stabellini
(?)
(?)
@ 2016-07-14 10:49 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:49 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Mark Rutland, devicetree, Michael Turquette, Stephen Boyd,
Dirk Behme, Julien Grall, xen-devel, linux-clk, linux-arm-kernel
On 14.07.2016 12:38, Stefano Stabellini wrote:
> 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.
Let me ask then: Do we have a practical example where it's not
sufficient practically?
To my understanding, Xen people have been happy with the
"clk_ignore_unused" workaround since ~2 years, now [1]. And I think the
"clk_ignore_unused" workaround does mainly the same like the patch
discussed here. It doesn't care regarding clk_set_rate from changing
properties, too?
While I agree that the patch theoretically incomplete, if nobody has a
real world example I would think that from practical point of view it's
sufficient in a first step.
If this is the case, I'd propose to fix the practical issue in a first
step with a patch (this one) which is sufficient to fix the issues the
Xen users have. And update the code for theoretical future issues in a
second step.
Best regards
Dirk
[1] http://bugs.xenproject.org/xen/bug/45
> 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?
>
> 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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:38 ` Stefano Stabellini
(?)
@ 2016-07-14 10:49 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:49 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Michael Turquette, Dirk Behme, Mark Rutland, devicetree,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 12:38, Stefano Stabellini wrote:
> 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.
Let me ask then: Do we have a practical example where it's not
sufficient practically?
To my understanding, Xen people have been happy with the
"clk_ignore_unused" workaround since ~2 years, now [1]. And I think the
"clk_ignore_unused" workaround does mainly the same like the patch
discussed here. It doesn't care regarding clk_set_rate from changing
properties, too?
While I agree that the patch theoretically incomplete, if nobody has a
real world example I would think that from practical point of view it's
sufficient in a first step.
If this is the case, I'd propose to fix the practical issue in a first
step with a patch (this one) which is sufficient to fix the issues the
Xen users have. And update the code for theoretical future issues in a
second step.
Best regards
Dirk
[1] http://bugs.xenproject.org/xen/bug/45
> 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?
>
> 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?
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:49 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:49 UTC (permalink / raw)
To: linux-arm-kernel
On 14.07.2016 12:38, Stefano Stabellini wrote:
> 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.
Let me ask then: Do we have a practical example where it's not
sufficient practically?
To my understanding, Xen people have been happy with the
"clk_ignore_unused" workaround since ~2 years, now [1]. And I think the
"clk_ignore_unused" workaround does mainly the same like the patch
discussed here. It doesn't care regarding clk_set_rate from changing
properties, too?
While I agree that the patch theoretically incomplete, if nobody has a
real world example I would think that from practical point of view it's
sufficient in a first step.
If this is the case, I'd propose to fix the practical issue in a first
step with a patch (this one) which is sufficient to fix the issues the
Xen users have. And update the code for theoretical future issues in a
second step.
Best regards
Dirk
[1] http://bugs.xenproject.org/xen/bug/45
> 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?
>
> 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?
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:49 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 10:49 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Michael Turquette, Dirk Behme, Mark Rutland, devicetree,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 12:38, Stefano Stabellini wrote:
> 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.
Let me ask then: Do we have a practical example where it's not
sufficient practically?
To my understanding, Xen people have been happy with the
"clk_ignore_unused" workaround since ~2 years, now [1]. And I think the
"clk_ignore_unused" workaround does mainly the same like the patch
discussed here. It doesn't care regarding clk_set_rate from changing
properties, too?
While I agree that the patch theoretically incomplete, if nobody has a
real world example I would think that from practical point of view it's
sufficient in a first step.
If this is the case, I'd propose to fix the practical issue in a first
step with a patch (this one) which is sufficient to fix the issues the
Xen users have. And update the code for theoretical future issues in a
second step.
Best regards
Dirk
[1] http://bugs.xenproject.org/xen/bug/45
> 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?
>
> 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?
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:49 ` Dirk Behme
(?)
(?)
@ 2016-07-14 15:55 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 15:55 UTC (permalink / raw)
To: Dirk Behme
Cc: Mark Rutland, devicetree, Stefano Stabellini, Michael Turquette,
Stephen Boyd, Dirk Behme, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 14.07.2016 12:38, Stefano Stabellini wrote:
> > 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.
>
>
> Let me ask then: Do we have a practical example where it's not sufficient
> practically?
>
> To my understanding, Xen people have been happy with the "clk_ignore_unused"
> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
> workaround does mainly the same like the patch discussed here. It doesn't care
> regarding clk_set_rate from changing properties, too?
Let me premise that I appreciate what you are trying to achieve with
this patch and I don't want to feature-creep it.
However we are defining a new Device Tree binding, one which will have
to be supported for a long time by both Xen and Linux, so at the very
least we need to have the full picture. We need to understand if the
binding if sufficient or if we need something different to solve the
problem completely.
Once we understand that, I am happy to accept a partial implementation
in Linux, as long as it is a step in the right direction. Does it make
sense?
> While I agree that the patch theoretically incomplete, if nobody has a real
> world example I would think that from practical point of view it's sufficient
> in a first step.
>
> If this is the case, I'd propose to fix the practical issue in a first step
> with a patch (this one) which is sufficient to fix the issues the Xen users
> have. And update the code for theoretical future issues in a second step.
>
> Best regards
>
> Dirk
>
> [1] http://bugs.xenproject.org/xen/bug/45
>
>
> > 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?
> >
> > 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?
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:49 ` Dirk Behme
@ 2016-07-14 15:55 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 15:55 UTC (permalink / raw)
To: Dirk Behme
Cc: Stefano Stabellini, Michael Turquette, Dirk Behme, Mark Rutland,
devicetree, Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 14.07.2016 12:38, Stefano Stabellini wrote:
> > 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.
>
>
> Let me ask then: Do we have a practical example where it's not sufficient
> practically?
>
> To my understanding, Xen people have been happy with the "clk_ignore_unused"
> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
> workaround does mainly the same like the patch discussed here. It doesn't care
> regarding clk_set_rate from changing properties, too?
Let me premise that I appreciate what you are trying to achieve with
this patch and I don't want to feature-creep it.
However we are defining a new Device Tree binding, one which will have
to be supported for a long time by both Xen and Linux, so at the very
least we need to have the full picture. We need to understand if the
binding if sufficient or if we need something different to solve the
problem completely.
Once we understand that, I am happy to accept a partial implementation
in Linux, as long as it is a step in the right direction. Does it make
sense?
> While I agree that the patch theoretically incomplete, if nobody has a real
> world example I would think that from practical point of view it's sufficient
> in a first step.
>
> If this is the case, I'd propose to fix the practical issue in a first step
> with a patch (this one) which is sufficient to fix the issues the Xen users
> have. And update the code for theoretical future issues in a second step.
>
> Best regards
>
> Dirk
>
> [1] http://bugs.xenproject.org/xen/bug/45
>
>
> > 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?
> >
> > 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?
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 15:55 ` Stefano Stabellini
0 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 15:55 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 14 Jul 2016, Dirk Behme wrote:
> On 14.07.2016 12:38, Stefano Stabellini wrote:
> > 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.
>
>
> Let me ask then: Do we have a practical example where it's not sufficient
> practically?
>
> To my understanding, Xen people have been happy with the "clk_ignore_unused"
> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
> workaround does mainly the same like the patch discussed here. It doesn't care
> regarding clk_set_rate from changing properties, too?
Let me premise that I appreciate what you are trying to achieve with
this patch and I don't want to feature-creep it.
However we are defining a new Device Tree binding, one which will have
to be supported for a long time by both Xen and Linux, so at the very
least we need to have the full picture. We need to understand if the
binding if sufficient or if we need something different to solve the
problem completely.
Once we understand that, I am happy to accept a partial implementation
in Linux, as long as it is a step in the right direction. Does it make
sense?
> While I agree that the patch theoretically incomplete, if nobody has a real
> world example I would think that from practical point of view it's sufficient
> in a first step.
>
> If this is the case, I'd propose to fix the practical issue in a first step
> with a patch (this one) which is sufficient to fix the issues the Xen users
> have. And update the code for theoretical future issues in a second step.
>
> Best regards
>
> Dirk
>
> [1] http://bugs.xenproject.org/xen/bug/45
>
>
> > 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?
> >
> > 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?
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 15:55 ` Stefano Stabellini
@ 2016-07-14 16:30 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 16:30 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Dirk Behme, Michael Turquette, Mark Rutland, devicetree,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 17:55, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Dirk Behme wrote:
>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>> 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.
>>
>>
>> Let me ask then: Do we have a practical example where it's not sufficient
>> practically?
>>
>> To my understanding, Xen people have been happy with the "clk_ignore_unused"
>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>> workaround does mainly the same like the patch discussed here. It doesn't care
>> regarding clk_set_rate from changing properties, too?
>
> Let me premise that I appreciate what you are trying to achieve with
> this patch and I don't want to feature-creep it.
>
> However we are defining a new Device Tree binding,
I don't think so.
We are just using the existing one
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
, pick it from other device tree nodes (e.g. serial, timer etc) and
add it to the hypervisor node. And then use this existing one with the
existing well defined clock API.
> one which will have
> to be supported for a long time by both Xen and Linux, so at the very
> least we need to have the full picture. We need to understand if the
> binding if sufficient
Even if it's not sufficient, you can't change it.
> or if we need something different to solve the
> problem completely.
You might need anything additionally. E.g. an extension of the Linux
kernel clock API to be able to modify the flags was proposed.
Best regards
Dirk
P.S.: I still would be interested if we do have a practical example
where it's not sufficient practically?
> Once we understand that, I am happy to accept a partial implementation
> in Linux, as long as it is a step in the right direction. Does it make
> sense?
>
>
>
>> While I agree that the patch theoretically incomplete, if nobody has a real
>> world example I would think that from practical point of view it's sufficient
>> in a first step.
>>
>> If this is the case, I'd propose to fix the practical issue in a first step
>> with a patch (this one) which is sufficient to fix the issues the Xen users
>> have. And update the code for theoretical future issues in a second step.
>>
>> Best regards
>>
>> Dirk
>>
>> [1] http://bugs.xenproject.org/xen/bug/45
>>
>>
>>> 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?
>>>
>>> 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?
>>
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 16:30 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 16:30 UTC (permalink / raw)
To: linux-arm-kernel
On 14.07.2016 17:55, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Dirk Behme wrote:
>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>> 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.
>>
>>
>> Let me ask then: Do we have a practical example where it's not sufficient
>> practically?
>>
>> To my understanding, Xen people have been happy with the "clk_ignore_unused"
>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>> workaround does mainly the same like the patch discussed here. It doesn't care
>> regarding clk_set_rate from changing properties, too?
>
> Let me premise that I appreciate what you are trying to achieve with
> this patch and I don't want to feature-creep it.
>
> However we are defining a new Device Tree binding,
I don't think so.
We are just using the existing one
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
, pick it from other device tree nodes (e.g. serial, timer etc) and
add it to the hypervisor node. And then use this existing one with the
existing well defined clock API.
> one which will have
> to be supported for a long time by both Xen and Linux, so at the very
> least we need to have the full picture. We need to understand if the
> binding if sufficient
Even if it's not sufficient, you can't change it.
> or if we need something different to solve the
> problem completely.
You might need anything additionally. E.g. an extension of the Linux
kernel clock API to be able to modify the flags was proposed.
Best regards
Dirk
P.S.: I still would be interested if we do have a practical example
where it's not sufficient practically?
> Once we understand that, I am happy to accept a partial implementation
> in Linux, as long as it is a step in the right direction. Does it make
> sense?
>
>
>
>> While I agree that the patch theoretically incomplete, if nobody has a real
>> world example I would think that from practical point of view it's sufficient
>> in a first step.
>>
>> If this is the case, I'd propose to fix the practical issue in a first step
>> with a patch (this one) which is sufficient to fix the issues the Xen users
>> have. And update the code for theoretical future issues in a second step.
>>
>> Best regards
>>
>> Dirk
>>
>> [1] http://bugs.xenproject.org/xen/bug/45
>>
>>
>>> 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?
>>>
>>> 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?
>>
>
^ permalink raw reply [flat|nested] 120+ messages in thread
[parent not found: <5787BE0B.8060504-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 16:30 ` Dirk Behme
(?)
@ 2016-07-14 17:14 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 17:14 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini
Cc: Dirk Behme, Michael Turquette, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd,
xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hello,
On 14/07/16 17:30, Dirk Behme wrote:
> On 14.07.2016 17:55, Stefano Stabellini wrote:
>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>> 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.
>>>
>>>
>>> Let me ask then: Do we have a practical example where it's not
>>> sufficient
>>> practically?
>>>
>>> To my understanding, Xen people have been happy with the
>>> "clk_ignore_unused"
>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>> workaround does mainly the same like the patch discussed here. It
>>> doesn't care
>>> regarding clk_set_rate from changing properties, too?
>>
>> Let me premise that I appreciate what you are trying to achieve with
>> this patch and I don't want to feature-creep it.
>>
>> However we are defining a new Device Tree binding,
>
>
> I don't think so.
>
> We are just using the existing one
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>
>
> , pick it from other device tree nodes (e.g. serial, timer etc) and add
> it to the hypervisor node. And then use this existing one with the
> existing well defined clock API.
>
>
>> one which will have
>> to be supported for a long time by both Xen and Linux, so at the very
>> least we need to have the full picture. We need to understand if the
>> binding if sufficient
>
>
> Even if it's not sufficient, you can't change it.
I think you misunderstood Stefano's comment. Whilst the clock bindings
is set in stone, the binding you are adding in this patch is not yet
fixed. There is no requirement to follow what was defined in
clock-bindings.txt. I agree it would be convenient, but as mentioned by
Stefano this will need to be supported for a long time by Xen, Linux,
and any other consumer (i.e BSD kernels). So we have to be careful on
how it has been defined.
I would wait the answer of Michael on Stefano's question before taking
any decision here.
>
>
>> or if we need something different to solve the
>> problem completely.
>
>
> You might need anything additionally. E.g. an extension of the Linux
> kernel clock API to be able to modify the flags was proposed.
The Linux kernel is not the only consumer of the device tree bindings.
This is also used by other OS such as FreeBSD where you might already be
able (I have not actually checked) to forbid a user to change the clock
rate.
>
> Best regards
>
> Dirk
>
> P.S.: I still would be interested if we do have a practical example
> where it's not sufficient practically?
Very easy. What does prevent a driver to change the clock rate? Nothing
but the flags mentioned by Michael. There are already drivers which
modify the clock rate, thankfully those clocks are not shared with the
UART for now.
But we cannot rule out that it will not be possible in the future. Think
about a clock that would be used by another guest (I know it is still
theoretical as we have not yet solved the problem).
Regards,
--
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 17:14 ` Julien Grall
0 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 17:14 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 14/07/16 17:30, Dirk Behme wrote:
> On 14.07.2016 17:55, Stefano Stabellini wrote:
>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>> 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.
>>>
>>>
>>> Let me ask then: Do we have a practical example where it's not
>>> sufficient
>>> practically?
>>>
>>> To my understanding, Xen people have been happy with the
>>> "clk_ignore_unused"
>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>> workaround does mainly the same like the patch discussed here. It
>>> doesn't care
>>> regarding clk_set_rate from changing properties, too?
>>
>> Let me premise that I appreciate what you are trying to achieve with
>> this patch and I don't want to feature-creep it.
>>
>> However we are defining a new Device Tree binding,
>
>
> I don't think so.
>
> We are just using the existing one
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>
>
> , pick it from other device tree nodes (e.g. serial, timer etc) and add
> it to the hypervisor node. And then use this existing one with the
> existing well defined clock API.
>
>
>> one which will have
>> to be supported for a long time by both Xen and Linux, so at the very
>> least we need to have the full picture. We need to understand if the
>> binding if sufficient
>
>
> Even if it's not sufficient, you can't change it.
I think you misunderstood Stefano's comment. Whilst the clock bindings
is set in stone, the binding you are adding in this patch is not yet
fixed. There is no requirement to follow what was defined in
clock-bindings.txt. I agree it would be convenient, but as mentioned by
Stefano this will need to be supported for a long time by Xen, Linux,
and any other consumer (i.e BSD kernels). So we have to be careful on
how it has been defined.
I would wait the answer of Michael on Stefano's question before taking
any decision here.
>
>
>> or if we need something different to solve the
>> problem completely.
>
>
> You might need anything additionally. E.g. an extension of the Linux
> kernel clock API to be able to modify the flags was proposed.
The Linux kernel is not the only consumer of the device tree bindings.
This is also used by other OS such as FreeBSD where you might already be
able (I have not actually checked) to forbid a user to change the clock
rate.
>
> Best regards
>
> Dirk
>
> P.S.: I still would be interested if we do have a practical example
> where it's not sufficient practically?
Very easy. What does prevent a driver to change the clock rate? Nothing
but the flags mentioned by Michael. There are already drivers which
modify the clock rate, thankfully those clocks are not shared with the
UART for now.
But we cannot rule out that it will not be possible in the future. Think
about a clock that would be used by another guest (I know it is still
theoretical as we have not yet solved the problem).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 17:14 ` Julien Grall
0 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 17:14 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini
Cc: Dirk Behme, Michael Turquette, Mark Rutland, devicetree,
Stephen Boyd, xen-devel, linux-clk, linux-arm-kernel
Hello,
On 14/07/16 17:30, Dirk Behme wrote:
> On 14.07.2016 17:55, Stefano Stabellini wrote:
>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>> 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.
>>>
>>>
>>> Let me ask then: Do we have a practical example where it's not
>>> sufficient
>>> practically?
>>>
>>> To my understanding, Xen people have been happy with the
>>> "clk_ignore_unused"
>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>> workaround does mainly the same like the patch discussed here. It
>>> doesn't care
>>> regarding clk_set_rate from changing properties, too?
>>
>> Let me premise that I appreciate what you are trying to achieve with
>> this patch and I don't want to feature-creep it.
>>
>> However we are defining a new Device Tree binding,
>
>
> I don't think so.
>
> We are just using the existing one
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>
>
> , pick it from other device tree nodes (e.g. serial, timer etc) and add
> it to the hypervisor node. And then use this existing one with the
> existing well defined clock API.
>
>
>> one which will have
>> to be supported for a long time by both Xen and Linux, so at the very
>> least we need to have the full picture. We need to understand if the
>> binding if sufficient
>
>
> Even if it's not sufficient, you can't change it.
I think you misunderstood Stefano's comment. Whilst the clock bindings
is set in stone, the binding you are adding in this patch is not yet
fixed. There is no requirement to follow what was defined in
clock-bindings.txt. I agree it would be convenient, but as mentioned by
Stefano this will need to be supported for a long time by Xen, Linux,
and any other consumer (i.e BSD kernels). So we have to be careful on
how it has been defined.
I would wait the answer of Michael on Stefano's question before taking
any decision here.
>
>
>> or if we need something different to solve the
>> problem completely.
>
>
> You might need anything additionally. E.g. an extension of the Linux
> kernel clock API to be able to modify the flags was proposed.
The Linux kernel is not the only consumer of the device tree bindings.
This is also used by other OS such as FreeBSD where you might already be
able (I have not actually checked) to forbid a user to change the clock
rate.
>
> Best regards
>
> Dirk
>
> P.S.: I still would be interested if we do have a practical example
> where it's not sufficient practically?
Very easy. What does prevent a driver to change the clock rate? Nothing
but the flags mentioned by Michael. There are already drivers which
modify the clock rate, thankfully those clocks are not shared with the
UART for now.
But we cannot rule out that it will not be possible in the future. Think
about a clock that would be used by another guest (I know it is still
theoretical as we have not yet solved the problem).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 17:14 ` Julien Grall
(?)
@ 2016-07-15 7:53 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-15 7:53 UTC (permalink / raw)
To: Julien Grall
Cc: Dirk Behme, Stefano Stabellini, Michael Turquette, Mark Rutland,
devicetree, Stephen Boyd, xen-devel, linux-clk, linux-arm-kernel
On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> 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.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.
It has to be
a) identical what we pick from UART, timer, etc
b) compatible to the kernel's clock API
No?
With these two requirements I have some difficulties to imagine how it
could be different to the clock binding from clock-bindings.txt?
> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).
Sounds like an additional argument for clock-bindings.txt ;)
> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.
Fine with me :)
Best regards
Dirk
>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-15 7:53 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-15 7:53 UTC (permalink / raw)
To: linux-arm-kernel
On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> 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.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.
It has to be
a) identical what we pick from UART, timer, etc
b) compatible to the kernel's clock API
No?
With these two requirements I have some difficulties to imagine how it
could be different to the clock binding from clock-bindings.txt?
> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).
Sounds like an additional argument for clock-bindings.txt ;)
> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.
Fine with me :)
Best regards
Dirk
>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-15 7:53 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-15 7:53 UTC (permalink / raw)
To: Julien Grall
Cc: Dirk Behme, Stefano Stabellini, Michael Turquette, Mark Rutland,
devicetree, Stephen Boyd, xen-devel, linux-clk, linux-arm-kernel
On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> 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.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.
It has to be
a) identical what we pick from UART, timer, etc
b) compatible to the kernel's clock API
No?
With these two requirements I have some difficulties to imagine how it
could be different to the clock binding from clock-bindings.txt?
> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).
Sounds like an additional argument for clock-bindings.txt ;)
> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.
Fine with me :)
Best regards
Dirk
>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 17:14 ` Julien Grall
` (2 preceding siblings ...)
(?)
@ 2016-07-15 7:53 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-15 7:53 UTC (permalink / raw)
To: Julien Grall
Cc: Mark Rutland, devicetree, Stefano Stabellini, Michael Turquette,
Stephen Boyd, Dirk Behme, xen-devel, linux-clk, linux-arm-kernel
On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> 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.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.
It has to be
a) identical what we pick from UART, timer, etc
b) compatible to the kernel's clock API
No?
With these two requirements I have some difficulties to imagine how it
could be different to the clock binding from clock-bindings.txt?
> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).
Sounds like an additional argument for clock-bindings.txt ;)
> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.
Fine with me :)
Best regards
Dirk
>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 16:30 ` Dirk Behme
(?)
(?)
@ 2016-07-14 17:14 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-14 17:14 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini
Cc: Mark Rutland, devicetree, Dirk Behme, Michael Turquette,
Stephen Boyd, xen-devel, linux-clk, linux-arm-kernel
Hello,
On 14/07/16 17:30, Dirk Behme wrote:
> On 14.07.2016 17:55, Stefano Stabellini wrote:
>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>> 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.
>>>
>>>
>>> Let me ask then: Do we have a practical example where it's not
>>> sufficient
>>> practically?
>>>
>>> To my understanding, Xen people have been happy with the
>>> "clk_ignore_unused"
>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>> workaround does mainly the same like the patch discussed here. It
>>> doesn't care
>>> regarding clk_set_rate from changing properties, too?
>>
>> Let me premise that I appreciate what you are trying to achieve with
>> this patch and I don't want to feature-creep it.
>>
>> However we are defining a new Device Tree binding,
>
>
> I don't think so.
>
> We are just using the existing one
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>
>
> , pick it from other device tree nodes (e.g. serial, timer etc) and add
> it to the hypervisor node. And then use this existing one with the
> existing well defined clock API.
>
>
>> one which will have
>> to be supported for a long time by both Xen and Linux, so at the very
>> least we need to have the full picture. We need to understand if the
>> binding if sufficient
>
>
> Even if it's not sufficient, you can't change it.
I think you misunderstood Stefano's comment. Whilst the clock bindings
is set in stone, the binding you are adding in this patch is not yet
fixed. There is no requirement to follow what was defined in
clock-bindings.txt. I agree it would be convenient, but as mentioned by
Stefano this will need to be supported for a long time by Xen, Linux,
and any other consumer (i.e BSD kernels). So we have to be careful on
how it has been defined.
I would wait the answer of Michael on Stefano's question before taking
any decision here.
>
>
>> or if we need something different to solve the
>> problem completely.
>
>
> You might need anything additionally. E.g. an extension of the Linux
> kernel clock API to be able to modify the flags was proposed.
The Linux kernel is not the only consumer of the device tree bindings.
This is also used by other OS such as FreeBSD where you might already be
able (I have not actually checked) to forbid a user to change the clock
rate.
>
> Best regards
>
> Dirk
>
> P.S.: I still would be interested if we do have a practical example
> where it's not sufficient practically?
Very easy. What does prevent a driver to change the clock rate? Nothing
but the flags mentioned by Michael. There are already drivers which
modify the clock rate, thankfully those clocks are not shared with the
UART for now.
But we cannot rule out that it will not be possible in the future. Think
about a clock that would be used by another guest (I know it is still
theoretical as we have not yet solved the problem).
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 15:55 ` Stefano Stabellini
(?)
(?)
@ 2016-07-14 16:30 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-14 16:30 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Mark Rutland, devicetree, Dirk Behme, Michael Turquette,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On 14.07.2016 17:55, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Dirk Behme wrote:
>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>> 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.
>>
>>
>> Let me ask then: Do we have a practical example where it's not sufficient
>> practically?
>>
>> To my understanding, Xen people have been happy with the "clk_ignore_unused"
>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>> workaround does mainly the same like the patch discussed here. It doesn't care
>> regarding clk_set_rate from changing properties, too?
>
> Let me premise that I appreciate what you are trying to achieve with
> this patch and I don't want to feature-creep it.
>
> However we are defining a new Device Tree binding,
I don't think so.
We are just using the existing one
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
, pick it from other device tree nodes (e.g. serial, timer etc) and
add it to the hypervisor node. And then use this existing one with the
existing well defined clock API.
> one which will have
> to be supported for a long time by both Xen and Linux, so at the very
> least we need to have the full picture. We need to understand if the
> binding if sufficient
Even if it's not sufficient, you can't change it.
> or if we need something different to solve the
> problem completely.
You might need anything additionally. E.g. an extension of the Linux
kernel clock API to be able to modify the flags was proposed.
Best regards
Dirk
P.S.: I still would be interested if we do have a practical example
where it's not sufficient practically?
> Once we understand that, I am happy to accept a partial implementation
> in Linux, as long as it is a step in the right direction. Does it make
> sense?
>
>
>
>> While I agree that the patch theoretically incomplete, if nobody has a real
>> world example I would think that from practical point of view it's sufficient
>> in a first step.
>>
>> If this is the case, I'd propose to fix the practical issue in a first step
>> with a patch (this one) which is sufficient to fix the issues the Xen users
>> have. And update the code for theoretical future issues in a second step.
>>
>> Best regards
>>
>> Dirk
>>
>> [1] http://bugs.xenproject.org/xen/bug/45
>>
>>
>>> 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?
>>>
>>> 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?
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:38 ` Stefano Stabellini
(?)
@ 2016-07-22 0:07 ` Michael Turquette
-1 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-22 0:07 UTC (permalink / raw)
To: Dirk Behme
Cc: Dirk Behme, Stefano Stabellini, Mark Rutland, devicetree,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-22 0:07 ` Michael Turquette
0 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-22 0:07 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-22 0:07 ` Michael Turquette
0 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-22 0:07 UTC (permalink / raw)
To: Stefano Stabellini, Dirk Behme
Cc: Dirk Behme, Stefano Stabellini, Mark Rutland, devicetree,
Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
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 X=
en, and
> > > > > > > > the OS
> > > > > > > > must not alter their state any way, such as disabling or ga=
ting 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 c=
hanging
> > > > > > > (clk_set_rate) or a parent from changing (clk_set_parent). Th=
e only
> > > > > > > way
> > > > > > > to do this currently would be to set the following flags on t=
he
> > > > > > > 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. Th=
erefore
> > > > > > no API
> > > > > > is exported for this.
> > > > > > =
> > > > > > I.e. if we need to set these flags, we have to do that in Xen w=
here 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 t=
he
> > > > 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 her=
e,
> > 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
>=20
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-22 0:07 ` Michael Turquette
@ 2016-07-22 1:16 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-22 1:16 UTC (permalink / raw)
To: Michael Turquette
Cc: Stefano Stabellini, Dirk Behme, Dirk Behme, Mark Rutland,
devicetree, Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On Thu, 21 Jul 2016, Michael Turquette wrote:
> 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.
All right. But could we design a new device tree binding which the Xen
hypervisor would use to politely ask the clock provider in Linux to set
CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
Xen would have to modify the DTB before booting Linux with the new
binding.
> > 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?
Who is usually the component with knowledge of the clock rate to set? If
it's a device driver, then neither the Xen hypervisor, nor the Xen core
drivers in Linux would know anything about it. (Unless the clock rate is
specified on device tree via assigned-clock-rates of course.)
> Is it correct that you would prefer some sort of never_touch_this_clk()
> api?
>From my understading, yes, never_touch_this_clk() would make things easier.
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-22 1:16 ` Stefano Stabellini
0 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-22 1:16 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 21 Jul 2016, Michael Turquette wrote:
> 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.
All right. But could we design a new device tree binding which the Xen
hypervisor would use to politely ask the clock provider in Linux to set
CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
Xen would have to modify the DTB before booting Linux with the new
binding.
> > 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?
Who is usually the component with knowledge of the clock rate to set? If
it's a device driver, then neither the Xen hypervisor, nor the Xen core
drivers in Linux would know anything about it. (Unless the clock rate is
specified on device tree via assigned-clock-rates of course.)
> Is it correct that you would prefer some sort of never_touch_this_clk()
> api?
>From my understading, yes, never_touch_this_clk() would make things easier.
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-22 1:16 ` Stefano Stabellini
(?)
@ 2016-07-27 5:05 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-27 5:05 UTC (permalink / raw)
To: Stefano Stabellini, Michael Turquette, Julien Grall
Cc: Mark Rutland, devicetree, Dirk Behme, Stephen Boyd, xen-devel,
linux-clk, linux-arm-kernel
Hi Michael, Stefano and Julien,
On 22.07.2016 03:16, Stefano Stabellini wrote:
> On Thu, 21 Jul 2016, Michael Turquette wrote:
>> 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.
>
> All right. But could we design a new device tree binding which the Xen
> hypervisor would use to politely ask the clock provider in Linux to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>
> Xen would have to modify the DTB before booting Linux with the new
> binding.
>
>
>>> 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?
>
> Who is usually the component with knowledge of the clock rate to set? If
> it's a device driver, then neither the Xen hypervisor, nor the Xen core
> drivers in Linux would know anything about it. (Unless the clock rate is
> specified on device tree via assigned-clock-rates of course.)
>
>
>> Is it correct that you would prefer some sort of never_touch_this_clk()
>> api?
>
>>From my understading, yes, never_touch_this_clk() would make things easier.
Would it be somehow worth to wait for anything like this
never_touch_this_clk() api? Or should we try to proceed with
clk_prepare_enable() like done in this patch for the moment?
Best regards
Dirk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-22 1:16 ` Stefano Stabellini
@ 2016-07-27 5:05 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-27 5:05 UTC (permalink / raw)
To: Stefano Stabellini, Michael Turquette, Julien Grall
Cc: Dirk Behme, Mark Rutland, devicetree, Stephen Boyd, xen-devel,
linux-clk, linux-arm-kernel
Hi Michael, Stefano and Julien,
On 22.07.2016 03:16, Stefano Stabellini wrote:
> On Thu, 21 Jul 2016, Michael Turquette wrote:
>> 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.
>
> All right. But could we design a new device tree binding which the Xen
> hypervisor would use to politely ask the clock provider in Linux to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>
> Xen would have to modify the DTB before booting Linux with the new
> binding.
>
>
>>> 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?
>
> Who is usually the component with knowledge of the clock rate to set? If
> it's a device driver, then neither the Xen hypervisor, nor the Xen core
> drivers in Linux would know anything about it. (Unless the clock rate is
> specified on device tree via assigned-clock-rates of course.)
>
>
>> Is it correct that you would prefer some sort of never_touch_this_clk()
>> api?
>
>>From my understading, yes, never_touch_this_clk() would make things easier.
Would it be somehow worth to wait for anything like this
never_touch_this_clk() api? Or should we try to proceed with
clk_prepare_enable() like done in this patch for the moment?
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-27 5:05 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-27 5:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi Michael, Stefano and Julien,
On 22.07.2016 03:16, Stefano Stabellini wrote:
> On Thu, 21 Jul 2016, Michael Turquette wrote:
>> 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.
>
> All right. But could we design a new device tree binding which the Xen
> hypervisor would use to politely ask the clock provider in Linux to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>
> Xen would have to modify the DTB before booting Linux with the new
> binding.
>
>
>>> 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?
>
> Who is usually the component with knowledge of the clock rate to set? If
> it's a device driver, then neither the Xen hypervisor, nor the Xen core
> drivers in Linux would know anything about it. (Unless the clock rate is
> specified on device tree via assigned-clock-rates of course.)
>
>
>> Is it correct that you would prefer some sort of never_touch_this_clk()
>> api?
>
>>From my understading, yes, never_touch_this_clk() would make things easier.
Would it be somehow worth to wait for anything like this
never_touch_this_clk() api? Or should we try to proceed with
clk_prepare_enable() like done in this patch for the moment?
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-27 5:05 ` Dirk Behme
@ 2016-07-28 11:17 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-28 11:17 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini, Michael Turquette
Cc: Dirk Behme, Mark Rutland, devicetree, Stephen Boyd, xen-devel,
linux-clk, linux-arm-kernel
Hi Dirk,
On 27/07/16 06:05, Dirk Behme wrote:
> Hi Michael, Stefano and Julien,
>
> On 22.07.2016 03:16, Stefano Stabellini wrote:
>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>> 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.
>>
>> All right. But could we design a new device tree binding which the Xen
>> hypervisor would use to politely ask the clock provider in Linux to set
>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>
>> Xen would have to modify the DTB before booting Linux with the new
>> binding.
>>
>>
>>>> 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?
>>
>> Who is usually the component with knowledge of the clock rate to set? If
>> it's a device driver, then neither the Xen hypervisor, nor the Xen core
>> drivers in Linux would know anything about it. (Unless the clock rate is
>> specified on device tree via assigned-clock-rates of course.)
>>
>>
>>> Is it correct that you would prefer some sort of never_touch_this_clk()
>>> api?
>>
>>> From my understading, yes, never_touch_this_clk() would make things
>>> easier.
>
>
> Would it be somehow worth to wait for anything like this
> never_touch_this_clk() api? Or should we try to proceed with
> clk_prepare_enable() like done in this patch for the moment?
I am not sure who will write the new api never_touch_this_clk(). Could
you suggest an implementation based on the discussion?
Regards,
>
> Best regards
>
> Dirk
>
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-28 11:17 ` Julien Grall
0 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-28 11:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dirk,
On 27/07/16 06:05, Dirk Behme wrote:
> Hi Michael, Stefano and Julien,
>
> On 22.07.2016 03:16, Stefano Stabellini wrote:
>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>> 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.
>>
>> All right. But could we design a new device tree binding which the Xen
>> hypervisor would use to politely ask the clock provider in Linux to set
>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>
>> Xen would have to modify the DTB before booting Linux with the new
>> binding.
>>
>>
>>>> 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?
>>
>> Who is usually the component with knowledge of the clock rate to set? If
>> it's a device driver, then neither the Xen hypervisor, nor the Xen core
>> drivers in Linux would know anything about it. (Unless the clock rate is
>> specified on device tree via assigned-clock-rates of course.)
>>
>>
>>> Is it correct that you would prefer some sort of never_touch_this_clk()
>>> api?
>>
>>> From my understading, yes, never_touch_this_clk() would make things
>>> easier.
>
>
> Would it be somehow worth to wait for anything like this
> never_touch_this_clk() api? Or should we try to proceed with
> clk_prepare_enable() like done in this patch for the moment?
I am not sure who will write the new api never_touch_this_clk(). Could
you suggest an implementation based on the discussion?
Regards,
>
> Best regards
>
> Dirk
>
--
Julien Grall
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-28 11:17 ` Julien Grall
@ 2016-07-28 14:35 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-28 14:35 UTC (permalink / raw)
To: Julien Grall, Michael Turquette
Cc: Stefano Stabellini, Dirk Behme, Mark Rutland, devicetree,
Stephen Boyd, xen-devel, linux-clk, linux-arm-kernel
On 28.07.2016 13:17, Julien Grall wrote:
> Hi Dirk,
>
> On 27/07/16 06:05, Dirk Behme wrote:
>> Hi Michael, Stefano and Julien,
>>
>> On 22.07.2016 03:16, Stefano Stabellini wrote:
>>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>>> 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.
>>>
>>> All right. But could we design a new device tree binding which the Xen
>>> hypervisor would use to politely ask the clock provider in Linux to
>>> set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>>
>>> Xen would have to modify the DTB before booting Linux with the new
>>> binding.
>>>
>>>
>>>>> 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?
>>>
>>> Who is usually the component with knowledge of the clock rate to
>>> set? If
>>> it's a device driver, then neither the Xen hypervisor, nor the Xen
>>> core
>>> drivers in Linux would know anything about it. (Unless the clock
>>> rate is
>>> specified on device tree via assigned-clock-rates of course.)
>>>
>>>
>>>> Is it correct that you would prefer some sort of
>>>> never_touch_this_clk()
>>>> api?
>>>
>>>> From my understading, yes, never_touch_this_clk() would make things
>>>> easier.
>>
>>
>> Would it be somehow worth to wait for anything like this
>> never_touch_this_clk() api? Or should we try to proceed with
>> clk_prepare_enable() like done in this patch for the moment?
>
> I am not sure who will write the new api never_touch_this_clk(). Could
> you suggest an implementation based on the discussion?
As this was a proposal from Michael, I'm hoping for Michael here,
somehow ;) At least for a hint if anything like never_touch_this_clk()
would be realistic to get accepted. And if so, how this could look like.
If this is unrealistic, I think we should go the proposed
clk_prepare_enable() way, as it seems this is the best we could do at
the moment without never_touch_this_clk().
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-28 14:35 ` Dirk Behme
0 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-28 14:35 UTC (permalink / raw)
To: linux-arm-kernel
On 28.07.2016 13:17, Julien Grall wrote:
> Hi Dirk,
>
> On 27/07/16 06:05, Dirk Behme wrote:
>> Hi Michael, Stefano and Julien,
>>
>> On 22.07.2016 03:16, Stefano Stabellini wrote:
>>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>>> 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.
>>>
>>> All right. But could we design a new device tree binding which the Xen
>>> hypervisor would use to politely ask the clock provider in Linux to
>>> set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>>
>>> Xen would have to modify the DTB before booting Linux with the new
>>> binding.
>>>
>>>
>>>>> 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?
>>>
>>> Who is usually the component with knowledge of the clock rate to
>>> set? If
>>> it's a device driver, then neither the Xen hypervisor, nor the Xen
>>> core
>>> drivers in Linux would know anything about it. (Unless the clock
>>> rate is
>>> specified on device tree via assigned-clock-rates of course.)
>>>
>>>
>>>> Is it correct that you would prefer some sort of
>>>> never_touch_this_clk()
>>>> api?
>>>
>>>> From my understading, yes, never_touch_this_clk() would make things
>>>> easier.
>>
>>
>> Would it be somehow worth to wait for anything like this
>> never_touch_this_clk() api? Or should we try to proceed with
>> clk_prepare_enable() like done in this patch for the moment?
>
> I am not sure who will write the new api never_touch_this_clk(). Could
> you suggest an implementation based on the discussion?
As this was a proposal from Michael, I'm hoping for Michael here,
somehow ;) At least for a hint if anything like never_touch_this_clk()
would be realistic to get accepted. And if so, how this could look like.
If this is unrealistic, I think we should go the proposed
clk_prepare_enable() way, as it seems this is the best we could do at
the moment without never_touch_this_clk().
Best regards
Dirk
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-28 11:17 ` Julien Grall
(?)
(?)
@ 2016-07-28 14:35 ` Dirk Behme
-1 siblings, 0 replies; 120+ messages in thread
From: Dirk Behme @ 2016-07-28 14:35 UTC (permalink / raw)
To: Julien Grall, Michael Turquette
Cc: Mark Rutland, devicetree, Dirk Behme, Stephen Boyd,
Stefano Stabellini, xen-devel, linux-clk, linux-arm-kernel
On 28.07.2016 13:17, Julien Grall wrote:
> Hi Dirk,
>
> On 27/07/16 06:05, Dirk Behme wrote:
>> Hi Michael, Stefano and Julien,
>>
>> On 22.07.2016 03:16, Stefano Stabellini wrote:
>>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>>> 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.
>>>
>>> All right. But could we design a new device tree binding which the Xen
>>> hypervisor would use to politely ask the clock provider in Linux to
>>> set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>>
>>> Xen would have to modify the DTB before booting Linux with the new
>>> binding.
>>>
>>>
>>>>> 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?
>>>
>>> Who is usually the component with knowledge of the clock rate to
>>> set? If
>>> it's a device driver, then neither the Xen hypervisor, nor the Xen
>>> core
>>> drivers in Linux would know anything about it. (Unless the clock
>>> rate is
>>> specified on device tree via assigned-clock-rates of course.)
>>>
>>>
>>>> Is it correct that you would prefer some sort of
>>>> never_touch_this_clk()
>>>> api?
>>>
>>>> From my understading, yes, never_touch_this_clk() would make things
>>>> easier.
>>
>>
>> Would it be somehow worth to wait for anything like this
>> never_touch_this_clk() api? Or should we try to proceed with
>> clk_prepare_enable() like done in this patch for the moment?
>
> I am not sure who will write the new api never_touch_this_clk(). Could
> you suggest an implementation based on the discussion?
As this was a proposal from Michael, I'm hoping for Michael here,
somehow ;) At least for a hint if anything like never_touch_this_clk()
would be realistic to get accepted. And if so, how this could look like.
If this is unrealistic, I think we should go the proposed
clk_prepare_enable() way, as it seems this is the best we could do at
the moment without never_touch_this_clk().
Best regards
Dirk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-27 5:05 ` Dirk Behme
(?)
(?)
@ 2016-07-28 11:17 ` Julien Grall
-1 siblings, 0 replies; 120+ messages in thread
From: Julien Grall @ 2016-07-28 11:17 UTC (permalink / raw)
To: Dirk Behme, Stefano Stabellini, Michael Turquette
Cc: Mark Rutland, devicetree, Dirk Behme, Stephen Boyd, xen-devel,
linux-clk, linux-arm-kernel
Hi Dirk,
On 27/07/16 06:05, Dirk Behme wrote:
> Hi Michael, Stefano and Julien,
>
> On 22.07.2016 03:16, Stefano Stabellini wrote:
>> On Thu, 21 Jul 2016, Michael Turquette wrote:
>>> 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.
>>
>> All right. But could we design a new device tree binding which the Xen
>> hypervisor would use to politely ask the clock provider in Linux to set
>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>>
>> Xen would have to modify the DTB before booting Linux with the new
>> binding.
>>
>>
>>>> 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?
>>
>> Who is usually the component with knowledge of the clock rate to set? If
>> it's a device driver, then neither the Xen hypervisor, nor the Xen core
>> drivers in Linux would know anything about it. (Unless the clock rate is
>> specified on device tree via assigned-clock-rates of course.)
>>
>>
>>> Is it correct that you would prefer some sort of never_touch_this_clk()
>>> api?
>>
>>> From my understading, yes, never_touch_this_clk() would make things
>>> easier.
>
>
> Would it be somehow worth to wait for anything like this
> never_touch_this_clk() api? Or should we try to proceed with
> clk_prepare_enable() like done in this patch for the moment?
I am not sure who will write the new api never_touch_this_clk(). Could
you suggest an implementation based on the discussion?
Regards,
>
> Best regards
>
> Dirk
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-22 0:07 ` Michael Turquette
` (2 preceding siblings ...)
(?)
@ 2016-07-22 1:16 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-22 1:16 UTC (permalink / raw)
To: Michael Turquette
Cc: Mark Rutland, devicetree, Dirk Behme, Stephen Boyd, Dirk Behme,
Julien Grall, Stefano Stabellini, xen-devel, linux-clk,
linux-arm-kernel
On Thu, 21 Jul 2016, Michael Turquette wrote:
> 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.
All right. But could we design a new device tree binding which the Xen
hypervisor would use to politely ask the clock provider in Linux to set
CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
Xen would have to modify the DTB before booting Linux with the new
binding.
> > 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?
Who is usually the component with knowledge of the clock rate to set? If
it's a device driver, then neither the Xen hypervisor, nor the Xen core
drivers in Linux would know anything about it. (Unless the clock rate is
specified on device tree via assigned-clock-rates of course.)
> Is it correct that you would prefer some sort of never_touch_this_clk()
> api?
From my understading, yes, never_touch_this_clk() would make things easier.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-14 10:38 ` Stefano Stabellini
` (4 preceding siblings ...)
(?)
@ 2016-07-22 0:07 ` Michael Turquette
-1 siblings, 0 replies; 120+ messages in thread
From: Michael Turquette @ 2016-07-22 0:07 UTC (permalink / raw)
To: Dirk Behme
Cc: Mark Rutland, devicetree, Stefano Stabellini, Stephen Boyd,
Dirk Behme, Julien Grall, xen-devel, linux-clk, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property
2016-07-13 21:03 ` Michael Turquette
@ 2016-07-14 10:25 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:25 UTC (permalink / raw)
To: Michael Turquette
Cc: Dirk Behme, Stefano Stabellini, Dirk Behme, Mark Rutland,
devicetree, Stephen Boyd, Julien Grall, xen-devel, linux-clk,
linux-arm-kernel
On Wed, 13 Jul 2016, 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.
Just for disambiguation, I guess you are referring to the Xen code in
Linux (such as arch/arm/xen/enlighten.c), right?
We have "Xen", the hypervisor, which runs before Linux and it is a
separate component running at EL2, and we have Xen support in Linux,
such as arch/arm/xen and drivers/xen. These are drivers to communicate
with the Xen hypervisor and other components in a Xen system.
In this case, the Xen hypervisor would be the owner of the clock. Xen
support in Linux, specifically arch/arm/xen/enlighten.c, only comes into
the picture to "warn" Linux that shouldn't really be messing with the
clocks. I guess that from a Linux clock driver point of view, it is a
consumer.
I hope that helps.
> > > 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@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/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@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> >
>
^ permalink raw reply [flat|nested] 120+ messages in thread* [Xen-devel] [PATCH v4] xen/arm: Add a clock property
@ 2016-07-14 10:25 ` Stefano Stabellini
0 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:25 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 13 Jul 2016, 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.
Just for disambiguation, I guess you are referring to the Xen code in
Linux (such as arch/arm/xen/enlighten.c), right?
We have "Xen", the hypervisor, which runs before Linux and it is a
separate component running at EL2, and we have Xen support in Linux,
such as arch/arm/xen and drivers/xen. These are drivers to communicate
with the Xen hypervisor and other components in a Xen system.
In this case, the Xen hypervisor would be the owner of the clock. Xen
support in Linux, specifically arch/arm/xen/enlighten.c, only comes into
the picture to "warn" Linux that shouldn't really be messing with the
clocks. I guess that from a Linux clock driver point of view, it is a
consumer.
I hope that helps.
> > > 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@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/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 at lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> >
>
^ permalink raw reply [flat|nested] 120+ messages in thread
* Re: [PATCH v4] xen/arm: Add a clock property
2016-07-13 21:03 ` Michael Turquette
` (4 preceding siblings ...)
(?)
@ 2016-07-14 10:25 ` Stefano Stabellini
-1 siblings, 0 replies; 120+ messages in thread
From: Stefano Stabellini @ 2016-07-14 10:25 UTC (permalink / raw)
To: Michael Turquette
Cc: Mark Rutland, devicetree, Dirk Behme, Stephen Boyd, Dirk Behme,
Julien Grall, Stefano Stabellini, xen-devel, linux-clk,
linux-arm-kernel
On Wed, 13 Jul 2016, 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.
Just for disambiguation, I guess you are referring to the Xen code in
Linux (such as arch/arm/xen/enlighten.c), right?
We have "Xen", the hypervisor, which runs before Linux and it is a
separate component running at EL2, and we have Xen support in Linux,
such as arch/arm/xen and drivers/xen. These are drivers to communicate
with the Xen hypervisor and other components in a Xen system.
In this case, the Xen hypervisor would be the owner of the clock. Xen
support in Linux, specifically arch/arm/xen/enlighten.c, only comes into
the picture to "warn" Linux that shouldn't really be messing with the
clocks. I guess that from a Linux clock driver point of view, it is a
consumer.
I hope that helps.
> > > 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@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/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@lists.xen.org
> > > https://lists.xen.org/xen-devel
> > >
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 120+ messages in thread