Re: [Xen-devel] [PATCH] xen/arm: domain_build: DT: add clocks node to the hypervisor node

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxx>
> ---
>   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,

--
Julien Grall

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