| Message ID | 1466504151-9163-1-git-send-email-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.06.2016 12:15, Dirk Behme wrote: > Some clocks might be used by Xen (drivers) and not by the Linux kernel. If > these are not registered by the Linux kernel, they might be disabled by the > Linux kernel's clk_disable_unused() as the kernel doesn't know that > they are used (by Xen drivers). The clock of the serial console handled by > Xen is one example for this. It might be disabled by clk_disable_unused() which > stops the whole serial output, even from Xen, then. > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 > > too. > > To fix this, add the clocks used by Xen to the hypervisor node. The Linux > kernel has to register the clocks from the hypervisor node, then. > > Therefore, collect all clocks from nodes used by Xen. These are marked > with DOMID_XEN. Afterwards, add a 'clocks' node to the hypervisor node > containing all these clocks. The Linux kernel can register all these clocks, > preventing them from being disabled, then. Just for the logs, the Linux kernel counterpart is http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438067.html Dirk > --- > xen/arch/arm/domain_build.c | 20 ++++++++++++++++++++ > xen/arch/arm/kernel.h | 7 +++++++ > 2 files changed, 27 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 2e4c295..fccf87e 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -657,6 +657,10 @@ static int make_hypervisor_node(const struct kernel_info *kinfo, > if ( res ) > return res; > > + res = fdt_property(fdt, "clocks", kinfo->clk.dtclocks, kinfo->clk.cnt); > + if ( res ) > + return res; > + > res = fdt_end_node(fdt); > > return res; > @@ -1213,9 +1217,11 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, > { /* sentinel */ }, > }; > struct dt_device_node *child; > + unsigned int len; > int res; > const char *name; > const char *path; > + const char *clocks; > > path = dt_node_full_name(node); > > @@ -1246,6 +1252,20 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, > if ( dt_device_used_by(node) == DOMID_XEN ) > { > DPRINT(" Skip it (used by Xen)\n"); > + > + /* > + * Remember the clock used by the skipped node > + * We add it later to the hypervisor node to make the > + * Linux kernel aware of its usage > + */ > + clocks = dt_get_property(node, "clocks", &len); > + if ( kinfo->clk.cnt + len >= MAX_DT_CLOCKS ) { > + printk("Failed to remember the clock node of %s\n", path); > + printk("Use the Linux kernel command 'clk_ignore_unused'\n"); > + return 0; > + } > + memcpy(&kinfo->clk.dtclocks[kinfo->clk.cnt], clocks, len); > + kinfo->clk.cnt += len; > return 0; > } > > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h > index c1b07d4..527b279 100644 > --- a/xen/arch/arm/kernel.h > +++ b/xen/arch/arm/kernel.h > @@ -10,6 +10,12 @@ > #include <xen/device_tree.h> > #include <asm/setup.h> > > +#define MAX_DT_CLOCKS 256 > +struct dtclk { > + unsigned char dtclocks[MAX_DT_CLOCKS]; > + unsigned int cnt; > +}; > + > struct kernel_info { > #ifdef CONFIG_ARM_64 > enum domain_type type; > @@ -18,6 +24,7 @@ struct kernel_info { > void *fdt; /* flat device tree */ > paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ > struct meminfo mem; > + struct dtclk clk; > > /* kernel entry point */ > paddr_t entry; >
Hello Dirk, On 21/06/16 11:15, Dirk Behme wrote: > Some clocks might be used by Xen (drivers) and not by the Linux kernel. If > these are not registered by the Linux kernel, they might be disabled by the > Linux kernel's clk_disable_unused() as the kernel doesn't know that > they are used (by Xen drivers). The clock of the serial console handled by > Xen is one example for this. It might be disabled by clk_disable_unused() which > stops the whole serial output, even from Xen, then. > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 > > too. > > To fix this, add the clocks used by Xen to the hypervisor node. The Linux > kernel has to register the clocks from the hypervisor node, then. > > Therefore, collect all clocks from nodes used by Xen. These are marked > with DOMID_XEN. Afterwards, add a 'clocks' node to the hypervisor node > containing all these clocks. The Linux kernel can register all these clocks, > preventing them from being disabled, then. > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > --- > xen/arch/arm/domain_build.c | 20 ++++++++++++++++++++ > xen/arch/arm/kernel.h | 7 +++++++ > 2 files changed, 27 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 2e4c295..fccf87e 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -657,6 +657,10 @@ static int make_hypervisor_node(const struct kernel_info *kinfo, > if ( res ) > return res; > > + res = fdt_property(fdt, "clocks", kinfo->clk.dtclocks, kinfo->clk.cnt); You may create an empty property if there is no clocks. > + if ( res ) > + return res; > + > res = fdt_end_node(fdt); > > return res; > @@ -1213,9 +1217,11 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, > { /* sentinel */ }, > }; > struct dt_device_node *child; > + unsigned int len; > int res; > const char *name; > const char *path; > + const char *clocks; > > path = dt_node_full_name(node); > > @@ -1246,6 +1252,20 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, > if ( dt_device_used_by(node) == DOMID_XEN ) > { > DPRINT(" Skip it (used by Xen)\n"); > + > + /* > + * Remember the clock used by the skipped node > + * We add it later to the hypervisor node to make the > + * Linux kernel aware of its usage > + */ > + clocks = dt_get_property(node, "clocks", &len); len is only updated when the property exists. > + if ( kinfo->clk.cnt + len >= MAX_DT_CLOCKS ) { The bracket should be a newline. > + printk("Failed to remember the clock node of %s\n", path); > + printk("Use the Linux kernel command 'clk_ignore_unused'\n"); > + return 0; I don't think this is tolerable. We need to fix it once and for all. I understand that xen does not provide a realloc function. Is there another way we can get a rid of this limit? I am wondering if we can use the member domain_list of dt_device_node to link the device having a clock property. And then latter one, allocate the memory + copying the data. > + } > + memcpy(&kinfo->clk.dtclocks[kinfo->clk.cnt], clocks, len); > + kinfo->clk.cnt += len; > return 0; > } > > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h > index c1b07d4..527b279 100644 > --- a/xen/arch/arm/kernel.h > +++ b/xen/arch/arm/kernel.h > @@ -10,6 +10,12 @@ > #include <xen/device_tree.h> > #include <asm/setup.h> > > +#define MAX_DT_CLOCKS 256 How did you define this value? > +struct dtclk { > + unsigned char dtclocks[MAX_DT_CLOCKS]; > + unsigned int cnt; > +}; > + > struct kernel_info { > #ifdef CONFIG_ARM_64 > enum domain_type type; > @@ -18,6 +24,7 @@ struct kernel_info { > void *fdt; /* flat device tree */ > paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ > struct meminfo mem; > + struct dtclk clk; > > /* kernel entry point */ > paddr_t entry; > Regards,
On 22/06/16 16:58, Julien Grall wrote: > On 21/06/16 11:15, Dirk Behme wrote: >> + printk("Failed to remember the clock node of %s\n", path); >> + printk("Use the Linux kernel command >> 'clk_ignore_unused'\n"); >> + return 0; > > I don't think this is tolerable. We need to fix it once and for all. > > I understand that xen does not provide a realloc function. Is there > another way we can get a rid of this limit? Note that I would wait that we agree on the device tree bindings before reworking this patch. It will avoid you to waste time if we decide to move towards a different solution. > I am wondering if we can use the member domain_list of dt_device_node to > link the device having a clock property. And then latter one, allocate > the memory + copying the data. > >> + } >> + memcpy(&kinfo->clk.dtclocks[kinfo->clk.cnt], clocks, len); >> + kinfo->clk.cnt += len; >> return 0; >> } >> Regards,
On 23.06.2016 17:02, Julien Grall wrote: > On 22/06/16 16:58, Julien Grall wrote: >> On 21/06/16 11:15, Dirk Behme wrote: >>> + printk("Failed to remember the clock node of %s\n", >>> path); >>> + printk("Use the Linux kernel command >>> 'clk_ignore_unused'\n"); >>> + return 0; >> >> I don't think this is tolerable. We need to fix it once and for all. >> >> I understand that xen does not provide a realloc function. Is there >> another way we can get a rid of this limit? > > Note that I would wait that we agree on the device tree bindings > before reworking this patch. You mean the 'clocks' binding for the hypervisor node? I.e. as proposed in https://lists.xen.org/archives/html/xen-devel/2016-06/msg02885.html create a patch for - linux/Documentation/devicetree/bindings/arm/xen.txt - xen/docs/misc/arm/device-tree/guest.txt and proceed only with the code changes when this is accepted? Best regards Dirk
Hi Dirk, On 23/06/16 16:59, Dirk Behme wrote: > On 23.06.2016 17:02, Julien Grall wrote: >> On 22/06/16 16:58, Julien Grall wrote: >>> On 21/06/16 11:15, Dirk Behme wrote: >>>> + printk("Failed to remember the clock node of %s\n", >>>> path); >>>> + printk("Use the Linux kernel command >>>> 'clk_ignore_unused'\n"); >>>> + return 0; >>> >>> I don't think this is tolerable. We need to fix it once and for all. >>> >>> I understand that xen does not provide a realloc function. Is there >>> another way we can get a rid of this limit? >> >> Note that I would wait that we agree on the device tree bindings >> before reworking this patch. > > > You mean the 'clocks' binding for the hypervisor node? I.e. as proposed in > > https://lists.xen.org/archives/html/xen-devel/2016-06/msg02885.html > > create a patch for > > - linux/Documentation/devicetree/bindings/arm/xen.txt > - xen/docs/misc/arm/device-tree/guest.txt > > and proceed only with the code changes when this is accepted? The second patch (xen/docs/misc/*) will be a verbatim copy of the first one. So writing only the first one is good for now. I will defer the implementation in Xen but keep the Linux one to give an overview to the maintainers (ARM and clocks) Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 2e4c295..fccf87e 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -657,6 +657,10 @@ static int make_hypervisor_node(const struct kernel_info *kinfo, if ( res ) return res; + res = fdt_property(fdt, "clocks", kinfo->clk.dtclocks, kinfo->clk.cnt); + if ( res ) + return res; + res = fdt_end_node(fdt); return res; @@ -1213,9 +1217,11 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, { /* sentinel */ }, }; struct dt_device_node *child; + unsigned int len; int res; const char *name; const char *path; + const char *clocks; path = dt_node_full_name(node); @@ -1246,6 +1252,20 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, if ( dt_device_used_by(node) == DOMID_XEN ) { DPRINT(" Skip it (used by Xen)\n"); + + /* + * Remember the clock used by the skipped node + * We add it later to the hypervisor node to make the + * Linux kernel aware of its usage + */ + clocks = dt_get_property(node, "clocks", &len); + if ( kinfo->clk.cnt + len >= MAX_DT_CLOCKS ) { + printk("Failed to remember the clock node of %s\n", path); + printk("Use the Linux kernel command 'clk_ignore_unused'\n"); + return 0; + } + memcpy(&kinfo->clk.dtclocks[kinfo->clk.cnt], clocks, len); + kinfo->clk.cnt += len; return 0; } diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h index c1b07d4..527b279 100644 --- a/xen/arch/arm/kernel.h +++ b/xen/arch/arm/kernel.h @@ -10,6 +10,12 @@ #include <xen/device_tree.h> #include <asm/setup.h> +#define MAX_DT_CLOCKS 256 +struct dtclk { + unsigned char dtclocks[MAX_DT_CLOCKS]; + unsigned int cnt; +}; + struct kernel_info { #ifdef CONFIG_ARM_64 enum domain_type type; @@ -18,6 +24,7 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; + struct dtclk clk; /* kernel entry point */ paddr_t entry;
Some clocks might be used by Xen (drivers) and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by the Linux kernel's clk_disable_unused() as the kernel doesn't know that they are used (by Xen drivers). The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 too. To fix this, add the clocks used by Xen to the hypervisor node. The Linux kernel has to register the clocks from the hypervisor node, then. Therefore, collect all clocks from nodes used by Xen. These are marked with DOMID_XEN. Afterwards, add a 'clocks' node to the hypervisor node containing all these clocks. The Linux kernel can register all these clocks, preventing them from being disabled, then. Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- xen/arch/arm/domain_build.c | 20 ++++++++++++++++++++ xen/arch/arm/kernel.h | 7 +++++++ 2 files changed, 27 insertions(+)