| Message ID | 1468309605-19522-1-git-send-email-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | Michael Turquette |
| Headers | show |
Quoting Dirk Behme (2016-07-12 00:46:45) > Clocks described by this property are reserved for use by Xen, and the OS > must not alter their state any way, such as disabling or gating a clock, > or modifying its rate. Ensuring this may impose constraints on parent > clocks or other resources used by the clock tree. Note that clk_prepare_enable will not prevent the rate from changing (clk_set_rate) or a parent from changing (clk_set_parent). The only way to do this currently would be to set the following flags on the effected clocks: CLK_SET_RATE_GATE CLK_SET_PARENT_GATE And then enable the clocks. All calls to clk_set_parent and clk_set_rate with those clocks in the path will fail, so long as they are prepared and enabled. This implementation detail is specific to Linux and definitely should not go into the binding. > > This property is used to proxy clocks for devices Xen has taken ownership > of, such as UARTs, for which the associated clock controller(s) remain > under the control of Dom0. I'm still not completely sure about this type of layering and whether or not it is sane. If you want Xen to manage the clock controller, AND you want Linux guests or dom0 to consume those clocks and manipulate them in other drivers, then this solution won't work. Regards, Mike > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 > > too. > > Signed-off-by: Dirk Behme <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) { } > -- > 2.8.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Best regards Dirk > 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) { } >> -- -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jul 2016, Dirk Behme wrote: > Clocks described by this property are reserved for use by Xen, and the OS > must not alter their state any way, such as disabling or gating a clock, > or modifying its rate. Ensuring this may impose constraints on parent > clocks or other resources used by the clock tree. > > This property is used to proxy clocks for devices Xen has taken ownership > of, such as UARTs, for which the associated clock controller(s) remain > under the control of Dom0. > > 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; > + } Given that this is a late initcall, the following should work: if (!xen_domain()) return -ENODEV; -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 13 Jul 2016, Dirk Behme wrote: > On 13.07.2016 00:26, Michael Turquette wrote: > > Quoting Dirk Behme (2016-07-12 00:46:45) > > > Clocks described by this property are reserved for use by Xen, and the OS > > > must not alter their state any way, such as disabling or gating a clock, > > > or modifying its rate. Ensuring this may impose constraints on parent > > > clocks or other resources used by the clock tree. > > > > Note that clk_prepare_enable will not prevent the rate from changing > > (clk_set_rate) or a parent from changing (clk_set_parent). The only way > > to do this currently would be to set the following flags on the effected > > clocks: > > > > CLK_SET_RATE_GATE > > CLK_SET_PARENT_GATE > > > > Regarding setting flags, I think we already talked about that. I think the > conclusion was that in our case its not possible to manipulate the flags in > the OS as this isn't intended to be done in cases like ours. Therefore no API > is exported for this. > > I.e. if we need to set these flags, we have to do that in Xen where we add the > clocks to the hypervisor node in the device tree. And not in the kernel patch > discussed here. These are internal Linux flags, aren't they? They cannot be set in Xen. If the only way to make sure that clk_set_rate and clk_set_parent fail on one of the clocks "owned" by Xen is to set CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE, we'll have to do that. Even if it means introducing a new internal Linux API. > > And then enable the clocks. All calls to clk_set_parent and clk_set_rate > > with those clocks in the path will fail, so long as they are prepared > > and enabled. This implementation detail is specific to Linux and > > definitely should not go into the binding. > > > > > > > > This property is used to proxy clocks for devices Xen has taken ownership > > > of, such as UARTs, for which the associated clock controller(s) remain > > > under the control of Dom0. > > > > I'm still not completely sure about this type of layering and whether or > > not it is sane. If you want Xen to manage the clock controller, AND you > > want Linux guests or dom0 to consume those clocks and manipulate them in > > other drivers, then this solution won't work. > > > > Regards, > > Mike > > > > > > > > Up to now, the workaround for this has been to use the Linux kernel > > > command line parameter 'clk_ignore_unused'. See Xen bug > > > > > > http://bugs.xenproject.org/xen/bug/45 > > > > > > too. > > > > > > Signed-off-by: Dirk Behme <dirk.behme@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) { } > > > -- > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13.07.2016 20:35, Stefano Stabellini wrote: > On Tue, 12 Jul 2016, Dirk Behme wrote: >> Clocks described by this property are reserved for use by Xen, and the OS >> must not alter their state any way, such as disabling or gating a clock, >> or modifying its rate. Ensuring this may impose constraints on parent >> clocks or other resources used by the clock tree. >> >> This property is used to proxy clocks for devices Xen has taken ownership >> of, such as UARTs, for which the associated clock controller(s) remain >> under the control of Dom0. >> >> 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; >> + } > > Given that this is a late initcall, the following should work: > > if (!xen_domain()) > return -ENODEV; Hmm, sorry, "should work" for what? We need the xen_node from device tree, anyway. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;) 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@lists.xen.org > https://lists.xen.org/xen-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 13 Jul 2016, Dirk Behme wrote: > On 13.07.2016 20:35, Stefano Stabellini wrote: > > On Tue, 12 Jul 2016, Dirk Behme wrote: > > > Clocks described by this property are reserved for use by Xen, and the OS > > > must not alter their state any way, such as disabling or gating a clock, > > > or modifying its rate. Ensuring this may impose constraints on parent > > > clocks or other resources used by the clock tree. > > > > > > This property is used to proxy clocks for devices Xen has taken ownership > > > of, such as UARTs, for which the associated clock controller(s) remain > > > under the control of Dom0. > > > > > > 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; > > > + } > > > > Given that this is a late initcall, the following should work: > > > > if (!xen_domain()) > > return -ENODEV; > > > Hmm, sorry, "should work" for what? As a Xen check, if (!xen_domain()) is the common pattern. > We need the xen_node from device tree, anyway. In that case, what don't you just use the global xen_node in this file? -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lists.xen.org > > https://lists.xen.org/xen-devel > > > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13.07.2016 21:07, Stefano Stabellini wrote: > On Wed, 13 Jul 2016, Dirk Behme wrote: >> On 13.07.2016 20:35, Stefano Stabellini wrote: >>> On Tue, 12 Jul 2016, Dirk Behme wrote: >>>> Clocks described by this property are reserved for use by Xen, and the OS >>>> must not alter their state any way, such as disabling or gating a clock, >>>> or modifying its rate. Ensuring this may impose constraints on parent >>>> clocks or other resources used by the clock tree. >>>> >>>> This property is used to proxy clocks for devices Xen has taken ownership >>>> of, such as UARTs, for which the associated clock controller(s) remain >>>> under the control of Dom0. >>>> >>>> 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; >>>> + } >>> >>> Given that this is a late initcall, the following should work: >>> >>> if (!xen_domain()) >>> return -ENODEV; >> >> >> Hmm, sorry, "should work" for what? > > As a Xen check, if (!xen_domain()) is the common pattern. > >> We need the xen_node from device tree, anyway. > > In that case, what don't you just use the global xen_node in this file? With the recent code there is no global xen_node any more: https://lists.xen.org/archives/html/xen-devel/2016-06/msg02878.html -- cut -- The code in this file has changed quite a lot with the support of ACPI. For instance "xen_node" is not anymore a global variable. Can you rebase your rework on top of the branch for-linus-4.8? You can find the branch in: git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git -- cut -- Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { } -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 14 Jul 2016, Dirk Behme wrote: > On 13.07.2016 21:07, Stefano Stabellini wrote: > > On Wed, 13 Jul 2016, Dirk Behme wrote: > > > On 13.07.2016 20:35, Stefano Stabellini wrote: > > > > On Tue, 12 Jul 2016, Dirk Behme wrote: > > > > > Clocks described by this property are reserved for use by Xen, and the > > > > > OS > > > > > must not alter their state any way, such as disabling or gating a > > > > > clock, > > > > > or modifying its rate. Ensuring this may impose constraints on parent > > > > > clocks or other resources used by the clock tree. > > > > > > > > > > This property is used to proxy clocks for devices Xen has taken > > > > > ownership > > > > > of, such as UARTs, for which the associated clock controller(s) remain > > > > > under the control of Dom0. > > > > > > > > > > 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; > > > > > + } > > > > > > > > Given that this is a late initcall, the following should work: > > > > > > > > if (!xen_domain()) > > > > return -ENODEV; > > > > > > > > > Hmm, sorry, "should work" for what? > > > > As a Xen check, if (!xen_domain()) is the common pattern. > > > > > We need the xen_node from device tree, anyway. > > > > In that case, what don't you just use the global xen_node in this file? > > > With the recent code there is no global xen_node any more: > > https://lists.xen.org/archives/html/xen-devel/2016-06/msg02878.html Ops, I was looking at upstream. I forgot about those. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? >> > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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). -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dirk, On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > Clocks described by this property are reserved for use by Xen, and the OS > must not alter their state any way, such as disabling or gating a clock, > or modifying its rate. Ensuring this may impose constraints on parent > clocks or other resources used by the clock tree. > > This property is used to proxy clocks for devices Xen has taken ownership > of, such as UARTs, for which the associated clock controller(s) remain > under the control of Dom0. I'm not familiar with using XEN at all, but I'm a bit puzzled... Can't you just add a clocks property to the (virtual) serial device node in DT? Then the (virtual) serial device driver can get and enable the clock? Alternatively, you can add a (virtual) clock controller, and power-domains and clock properties to all affected devices (I assume there can be others, besides virtual UARTs?), and let it be handled by Runtime PM, without the (virtual) device drivers having to care about clocks at all. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/07/16 10:43, Geert Uytterhoeven wrote: > Hi Dirk, Hi Geert, > On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: >> Clocks described by this property are reserved for use by Xen, and the OS >> must not alter their state any way, such as disabling or gating a clock, >> or modifying its rate. Ensuring this may impose constraints on parent >> clocks or other resources used by the clock tree. >> >> This property is used to proxy clocks for devices Xen has taken ownership >> of, such as UARTs, for which the associated clock controller(s) remain >> under the control of Dom0. > > I'm not familiar with using XEN at all, but I'm a bit puzzled... > > Can't you just add a clocks property to the (virtual) serial device node in DT? > Then the (virtual) serial device driver can get and enable the clock? There is no DT node for the Xen console (hvc). The UART used by Xen will be completely removed from the Device tree. > > Alternatively, you can add a (virtual) clock controller, and power-domains > and clock properties to all affected devices (I assume there can be others, > besides virtual UARTs?), and let it be handled by Runtime PM, without the > (virtual) device drivers having to care about clocks at all. I am not sure to understand here. The problem is not because we provide virtual device to DOM0 but because the physical devices will be completely hidden (i.e remove from the DT) from DOM0. Those devices will be removed from the Device Tree and may not have a corresponding virtual device. So we cannot replicate the clocks property in the device. In a previous mail [1], Stefano suggested to add a new property for the clock to mention that the clock should not changed (i.e rate, disable...). How would that fit? Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg01614.html
Hi Julien, On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote: > On 20/07/16 10:43, Geert Uytterhoeven wrote: >> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> >> wrote: >>> >>> Clocks described by this property are reserved for use by Xen, and the OS >>> must not alter their state any way, such as disabling or gating a clock, >>> or modifying its rate. Ensuring this may impose constraints on parent >>> clocks or other resources used by the clock tree. >>> >>> This property is used to proxy clocks for devices Xen has taken ownership >>> of, such as UARTs, for which the associated clock controller(s) remain >>> under the control of Dom0. >> >> >> I'm not familiar with using XEN at all, but I'm a bit puzzled... >> >> Can't you just add a clocks property to the (virtual) serial device node >> in DT? >> Then the (virtual) serial device driver can get and enable the clock? > > There is no DT node for the Xen console (hvc). The UART used by Xen will be > completely removed from the Device tree. Why is it removed? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/07/16 12:49, Geert Uytterhoeven wrote: > Hi Julien, > > On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote: >> On 20/07/16 10:43, Geert Uytterhoeven wrote: >>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> >>> wrote: >>>> >>>> Clocks described by this property are reserved for use by Xen, and the OS >>>> must not alter their state any way, such as disabling or gating a clock, >>>> or modifying its rate. Ensuring this may impose constraints on parent >>>> clocks or other resources used by the clock tree. >>>> >>>> This property is used to proxy clocks for devices Xen has taken ownership >>>> of, such as UARTs, for which the associated clock controller(s) remain >>>> under the control of Dom0. >>> >>> >>> I'm not familiar with using XEN at all, but I'm a bit puzzled... >>> >>> Can't you just add a clocks property to the (virtual) serial device node >>> in DT? >>> Then the (virtual) serial device driver can get and enable the clock? >> >> There is no DT node for the Xen console (hvc). The UART used by Xen will be >> completely removed from the Device tree. > > Why is it removed? Because the device is used exclusively by Xen and DOM0 should not touch it at all (IRQs and MMIOs are not mapped). Regards,
Hi Julien, On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote: > On 20/07/16 12:49, Geert Uytterhoeven wrote: >> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> On 20/07/16 10:43, Geert Uytterhoeven wrote: >>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> >>>> wrote: >>>>> Clocks described by this property are reserved for use by Xen, and the >>>>> OS >>>>> must not alter their state any way, such as disabling or gating a >>>>> clock, >>>>> or modifying its rate. Ensuring this may impose constraints on parent >>>>> clocks or other resources used by the clock tree. >>>>> >>>>> This property is used to proxy clocks for devices Xen has taken >>>>> ownership >>>>> of, such as UARTs, for which the associated clock controller(s) remain >>>>> under the control of Dom0. >>>> >>>> I'm not familiar with using XEN at all, but I'm a bit puzzled... >>>> >>>> Can't you just add a clocks property to the (virtual) serial device node >>>> in DT? >>>> Then the (virtual) serial device driver can get and enable the clock? >>> >>> There is no DT node for the Xen console (hvc). The UART used by Xen will >>> be >>> completely removed from the Device tree. >> >> Why is it removed? > > Because the device is used exclusively by Xen and DOM0 should not touch it > at all (IRQs and MMIOs are not mapped). IMHO then it's Xen's responsability to make sure not to disable the clock(s). Who removes the device node from the DT? If Xen, can't it just remember which clocks were present in removed device nodes? What to do on SoCs where the serial device is part of a power area (e.g. Renesas SH/R-Mobile)? Who will make sure it is not powered down? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20.07.2016 14:46, Geert Uytterhoeven wrote: > Hi Julien, > > On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote: >> On 20/07/16 12:49, Geert Uytterhoeven wrote: >>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> >>> wrote: >>>> On 20/07/16 10:43, Geert Uytterhoeven wrote: >>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> >>>>> wrote: >>>>>> Clocks described by this property are reserved for use by Xen, and the >>>>>> OS >>>>>> must not alter their state any way, such as disabling or gating a >>>>>> clock, >>>>>> or modifying its rate. Ensuring this may impose constraints on parent >>>>>> clocks or other resources used by the clock tree. >>>>>> >>>>>> This property is used to proxy clocks for devices Xen has taken >>>>>> ownership >>>>>> of, such as UARTs, for which the associated clock controller(s) remain >>>>>> under the control of Dom0. >>>>> >>>>> I'm not familiar with using XEN at all, but I'm a bit puzzled... >>>>> >>>>> Can't you just add a clocks property to the (virtual) serial device node >>>>> in DT? >>>>> Then the (virtual) serial device driver can get and enable the clock? >>>> >>>> There is no DT node for the Xen console (hvc). The UART used by Xen will >>>> be >>>> completely removed from the Device tree. >>> >>> Why is it removed? >> >> Because the device is used exclusively by Xen and DOM0 should not touch it >> at all (IRQs and MMIOs are not mapped). > > IMHO then it's Xen's responsability to make sure not to disable the clock(s). > > Who removes the device node from the DT? If Xen, can't it just remember > which clocks were present in removed device nodes? Yes, we are trying this with "moving" the clocks of the removed nodes to the Xen hypervisor node: https://lists.xen.org/archives/html/xen-devel/2016-06/msg02605.html The question discussed here is how to deal with these clocks properly in Linux, then. Best regards Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/07/2016 13:46, Geert Uytterhoeven wrote: > Hi Julien, Hello Geert, > On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote: >> On 20/07/16 12:49, Geert Uytterhoeven wrote: >>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> >>> wrote: >>>> On 20/07/16 10:43, Geert Uytterhoeven wrote: >>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> >>>>> wrote: >>>>>> Clocks described by this property are reserved for use by Xen, and the >>>>>> OS >>>>>> must not alter their state any way, such as disabling or gating a >>>>>> clock, >>>>>> or modifying its rate. Ensuring this may impose constraints on parent >>>>>> clocks or other resources used by the clock tree. >>>>>> >>>>>> This property is used to proxy clocks for devices Xen has taken >>>>>> ownership >>>>>> of, such as UARTs, for which the associated clock controller(s) remain >>>>>> under the control of Dom0. >>>>> >>>>> I'm not familiar with using XEN at all, but I'm a bit puzzled... >>>>> >>>>> Can't you just add a clocks property to the (virtual) serial device node >>>>> in DT? >>>>> Then the (virtual) serial device driver can get and enable the clock? >>>> >>>> There is no DT node for the Xen console (hvc). The UART used by Xen will >>>> be >>>> completely removed from the Device tree. >>> >>> Why is it removed? >> >> Because the device is used exclusively by Xen and DOM0 should not touch it >> at all (IRQs and MMIOs are not mapped). > > IMHO then it's Xen's responsability to make sure not to disable the clock(s). Xen does not disable the clock(s). The problem is Linux thinks the clock is not used and therefore will disable the clock. Actually Xen does not have any knowledge how to handle clocks (i.e setting rate, enabling/disabling). I would recommend you to read the binding description in the patch and not the implementation which is IHMO misleading. > > Who removes the device node from the DT? If Xen, can't it just remember > which clocks were present in removed device nodes? Xen is removing the node from the DT. Not removing the node will not change the problem. Linux is disabling the clock because there is no driver which used those clocks. As the clock subsystem will disable any unused clocks (from Linux point of view), the UART will get disabled and the console will be lost. What we want to achieve with this patch is to let Linux knows that this clock is in use by someone else (hypervisor or guest) and: * If the clock is not shared: don't disable it * If the clock is shared: don't change the rate > > What to do on SoCs where the serial device is part of a power area (e.g. > Renesas SH/R-Mobile)? Who will make sure it is not powered down? I don't have any knowledge on the Renesas SH/R-Mobile. However, we expect some cooperation between DOM0 and Xen to handle the power. For instance managing the clocks in Xen would require to implement clock driver for each SOC because it does not seem to have a generic way to do it. Given that Linux already knows a lot about the clock, we want to hand over the clock control to the hardware domain (i.e dom0). This means that we have to find a way to cooperate with it to keep enable clocks that we care about. Regards,
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Julien Grall (2016-07-20 06:21:45) > > > On 20/07/2016 13:46, Geert Uytterhoeven wrote: > > Hi Julien, > > Hello Geert, > > > On Wed, Jul 20, 2016 at 2:10 PM, Julien Grall <julien.grall@arm.com> wrote: > >> On 20/07/16 12:49, Geert Uytterhoeven wrote: > >>> On Wed, Jul 20, 2016 at 1:01 PM, Julien Grall <julien.grall@arm.com> > >>> wrote: > >>>> On 20/07/16 10:43, Geert Uytterhoeven wrote: > >>>>> On Tue, Jul 12, 2016 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> > >>>>> wrote: > >>>>>> Clocks described by this property are reserved for use by Xen, and the > >>>>>> OS > >>>>>> must not alter their state any way, such as disabling or gating a > >>>>>> clock, > >>>>>> or modifying its rate. Ensuring this may impose constraints on parent > >>>>>> clocks or other resources used by the clock tree. > >>>>>> > >>>>>> This property is used to proxy clocks for devices Xen has taken > >>>>>> ownership > >>>>>> of, such as UARTs, for which the associated clock controller(s) remain > >>>>>> under the control of Dom0. > >>>>> > >>>>> I'm not familiar with using XEN at all, but I'm a bit puzzled... > >>>>> > >>>>> Can't you just add a clocks property to the (virtual) serial device node > >>>>> in DT? > >>>>> Then the (virtual) serial device driver can get and enable the clock? > >>>> > >>>> There is no DT node for the Xen console (hvc). The UART used by Xen will > >>>> be > >>>> completely removed from the Device tree. > >>> > >>> Why is it removed? > >> > >> Because the device is used exclusively by Xen and DOM0 should not touch it > >> at all (IRQs and MMIOs are not mapped). > > > > IMHO then it's Xen's responsability to make sure not to disable the clock(s). > > Xen does not disable the clock(s). The problem is Linux thinks the clock > is not used and therefore will disable the clock. > > Actually Xen does not have any knowledge how to handle clocks (i.e > setting rate, enabling/disabling). I would recommend you to read the > binding description in the patch and not the implementation which is > IHMO misleading. > > > > > Who removes the device node from the DT? If Xen, can't it just remember > > which clocks were present in removed device nodes? > > Xen is removing the node from the DT. Not removing the node will not > change the problem. Linux is disabling the clock because there is no > driver which used those clocks. > > As the clock subsystem will disable any unused clocks (from Linux point > of view), the UART will get disabled and the console will be lost. > > What we want to achieve with this patch is to let Linux knows that this > clock is in use by someone else (hypervisor or guest) and: > * If the clock is not shared: don't disable it > * If the clock is shared: don't change the rate Thanks for the explanation above. It helped clarify things for me. So, can you have some sort of dummy driver in Linux that claims the resources needed by Xen and calls the necessary Linux apis to insure that Xen's needs are satisfied? As I mentioned in my previous reply from a couple of minutes ago: struct clk *clk = clk_get(xen_dev, ...); clk_prepare_enable(clk); clk_set_rate_range(...); I'm not sure where the rate info should come from, but this represents a correct use of clk consumer apis. Regards, Mike > > > > > What to do on SoCs where the serial device is part of a power area (e.g. > > Renesas SH/R-Mobile)? Who will make sure it is not powered down? > > I don't have any knowledge on the Renesas SH/R-Mobile. However, we > expect some cooperation between DOM0 and Xen to handle the power. > > For instance managing the clocks in Xen would require to implement clock > driver for each SOC because it does not seem to have a generic way to do it. > > Given that Linux already knows a lot about the clock, we want to hand > over the clock control to the hardware domain (i.e dom0). This means > that we have to find a way to cooperate with it to keep enable clocks > that we care about. > > Regards, > > -- > Julien Grall -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { }
Clocks described by this property are reserved for use by Xen, and the OS must not alter their state any way, such as disabling or gating a clock, or modifying its rate. Ensuring this may impose constraints on parent clocks or other resources used by the clock tree. This property is used to proxy clocks for devices Xen has taken ownership of, such as UARTs, for which the associated clock controller(s) remain under the control of Dom0. 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(+)