Re: [RFC PATCH 09/13] arch/arm: create device tree nodes for Dom0 cpufreq cpu driver

[Julien Grall <julien.grall@linaro.org>]

Hi Oleksandr,

On 10/07/2014 03:19 PM, Oleksandr Dmytryshyn wrote:
> This patch copies all cpu@0..cpu@N nodes (from input
> device tree) with properties to /cpus/cpu@0/private_data
> node (device tree for Dom0). Thus we can have any number
> of VCPUs in Dom0 and we give all information about all
> physical CPUs in the private_data node. Driver in Dom0
> should parse /cpus/cpu@0/private_data path instead of the
> /cpus path in the device tree.
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> ---
>  xen/arch/arm/domain_build.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2db0236..a142cc4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -436,6 +436,10 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>      char buf[15];
>      u32 clock_frequency;
>      bool_t clock_valid;
> +#ifdef HAS_CPUFREQ
> +    const struct dt_property *pp;
> +    char *node_name;
> +#endif
>  
>      DPRINT("Create cpus node\n");
>  
> @@ -517,6 +521,60 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>                  return res;
>          }
>  

The function make_cpus_node is quite big. I would introduce a new
function to copy the "private_data".

I was also thinking if it wouldn't have been better to move those nodes
under the hypervisor one because it's Xen stuff.

> +#ifdef HAS_CPUFREQ
> +        /*
> +         * Add "private_data" node to cpu@0 and copy to it
> +         * original cpu@0..cpu@N nodes with its properties.
> +         * This is needed for the cpufreq cpu driver in Dom0
> +         */

This copy is mostly related to Xen
Wouldn't it be better to copy those nodes in the hypervisor node?

> +        if ( cpu == 0 )
> +        {
> +            DPRINT("Create private_data node\n");
> +
> +            res = fdt_begin_node(fdt, "private_data");
> +            if ( res )
> +                return res;
> +
> +            dt_for_each_child_node( cpus, npcpu )
> +            {
> +                if ( dt_device_type_is_equal(npcpu, "cpu") )
> +                {
> +                    node_name = strrchr(dt_node_full_name(npcpu), '/');
> +                    if ( node_name )
> +                        node_name++;

IIRC, it's not possible to have an empty node_name here. The node name
will always be /cpus/name.

I would turn this if to an ASSERT, and drop the if just below.

> +
> +                    if ( node_name && *node_name )
> +                    {

Regards,

-- 
Julien Grall