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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Tue, Oct 7, 2014 at 6:26 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Oleksandr,
>
> On 10/07/2014 03:19 PM, Oleksandr Dmytryshyn wrote:
>> This patch copies all cpu@xxxxxx@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@xxxxxxxxxxxxxxx>
>> ---
>>  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.
I'll do it in the next patch-set.

>> +#ifdef HAS_CPUFREQ
>> +        /*
>> +         * Add "private_data" node to cpu@0 and copy to it
>> +         * original cpu@xxxxxx@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?
I'll do it in the next patch-set.

>> +        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'll do it in the next patch-set.

> I would turn this if to an ASSERT, and drop the if just below.
>
>> +
>> +                    if ( node_name && *node_name )
>> +                    {
>
> Regards,
>
> --
> Julien Grall

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