Re: [RFC PATCH v3 07/12] arch/arm: create device tree nodes for hwdom cpufreq cpu driver

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

Hi Oleksandr,

On 10/23/2014 04:07 PM, Oleksandr Dmytryshyn wrote:
> This patch copies all cpu@0..cpu@N nodes (from input
> device tree) with properties to /hypervisor/pcpus
> node (device tree for hwdom). Thus we can give all
> information about all physical CPUs in the pcpus node.
> Driver in hwdom should parse /hypervisor/pcpus 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 | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2db0236..2186514 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -326,6 +326,69 @@ static int make_memory_node(const struct domain *d,
>      return res;
>  }
>  
> +#ifdef HAS_CPUFREQ
> +static int fdt_copy_phys_cpus_nodes(void *fdt)
> +{
> +    int res;
> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    const struct dt_device_node *npcpu;
> +    const struct dt_property *pp;
> +    char *node_name;
> +
> +    if ( !cpus )
> +    {
> +        dprintk(XENLOG_ERR, "Missing /cpus node in the device tree?\n");
> +        return -ENOENT;
> +    }
> +
> +    /*
> +     * create pcpus node and copy to it
> +     * original cpu@0..cpu@N nodes with its properties.
> +     * This is needed for the cpufreq cpu driver in Dom0
> +     */
> +    DPRINT("Create pcpus node\n");
> +
> +    res = fdt_begin_node(fdt, "pcpus");

This new bindings has to be documented somewhere. The best places would
be in Documentation/devicetree/bindins/arm/xen.txt (see Linux repo).

> +    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), '/');
> +            node_name++;
> +
> +            ASSERT(node_name && *node_name);

The first check on node_name is pointless because of the node_name++ above.

I would divide the ASSERT in 2 parts, and do the first check before
node_name++;

> +
> +            DPRINT("Copy %s node to the pcpus\n", node_name);
> +
> +            res = fdt_begin_node(fdt, node_name);
> +            if ( res )
> +                return res;
> +
> +            dt_for_each_property_node( npcpu, pp )
> +            {
> +                if ( pp->length )
> +                {
> +                    res = fdt_property(fdt, pp->name, pp->value,
> +                                        pp->length);
> +                    if ( res )
> +                        return res;
> +                }
> +            }
> +

You can use write_properties to replace this loop.

> +            res = fdt_end_node(fdt);
> +            if ( res )
> +                return res;
> +        }
> +    }
> +
> +    res = fdt_end_node(fdt);
> +    return res;
> +}
> +#endif /* HAS_CPUFREQ */
> +
>  static int make_hypervisor_node(struct domain *d,
>                                  void *fdt, const struct dt_device_node *parent)
>  {
> @@ -386,6 +449,10 @@ static int make_hypervisor_node(struct domain *d,
>      if ( res )
>          return res;
>  
> +    #ifdef HAS_CPUFREQ
> +    fdt_copy_phys_cpus_nodes(fdt);

You forgot to check the return value of the function.

Regards,

-- 
Julien Grall