Re: [PATCH V1 27/29] xen/dts: device_get_reg: cells are 32 bits big endian value

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

On 09/09/2013 12:57 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> Device tree cells are 32-bit big endian value. Use __be32 to avoid confusion
>> later.
> 
> These are only used by sparse, right? So we wouldn't expect this to
> actually save us from any mistakes unless we make Xen support the use of
> sparse? (IOW I shouldn't rely on this and not think about this too hard
> during patch review because the compiler will get it right for me)

Rigth. I think it also helps the developer to know that value may not
have the same endianess as the processor.

>> Also replace get_val call by dt_next_cell.
> 
> Would prefer this separately.
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> (on both patches if you split)

I will split the patch in 2 parts.

> 
>> ---
>>  xen/common/device_tree.c |   28 +++++++---------------------
>>  1 file changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index b120585..4bc1ce4 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -154,25 +154,11 @@ static bool_t __init device_tree_node_compatible(const void *fdt, int node,
>>      return 0;
>>  }
>>  
>> -static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>> -{
>> -    *val = 0;
>> -
>> -    if ( cells > 2 )
>> -        early_panic("dtb value contains > 2 cells\n");
>> -
>> -    while ( cells-- )
>> -    {
>> -        *val <<= 32;
>> -        *val |= fdt32_to_cpu(*(*cell)++);
>> -    }
>> -}
>> -
>> -static void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
>> +static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>>                                         u32 size_cells, u64 *start, u64 *size)
>>  {
>> -    get_val(cell, address_cells, start);
>> -    get_val(cell, size_cells, size);
>> +    *start = dt_next_cell(address_cells, cell);
>> +    *size = dt_next_cell(size_cells, cell);
>>  }
>>  
>>  void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
>> @@ -327,7 +313,7 @@ static void __init process_memory_node(const void *fdt, int node,
>>      const struct fdt_property *prop;
>>      int i;
>>      int banks;
>> -    const u32 *cell;
>> +    const __be32 *cell;
>>      paddr_t start, size;
>>      u32 reg_cells = address_cells + size_cells;
>>  
>> @@ -345,7 +331,7 @@ static void __init process_memory_node(const void *fdt, int node,
>>          return;
>>      }
>>  
>> -    cell = (const u32 *)prop->data;
>> +    cell = (const __be32 *)prop->data;
>>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>  
>>      for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
>> @@ -396,7 +382,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>                                            u32 address_cells, u32 size_cells)
>>  {
>>      const struct fdt_property *prop;
>> -    const u32 *cell;
>> +    const __be32 *cell;
>>      int nr;
>>      struct dt_mb_module *mod;
>>      int len;
>> @@ -418,7 +404,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>          early_panic("fdt: node `%s': `reg` property length is too short\n",
>>                      name);
>>  
>> -    cell = (const u32 *)prop->data;
>> +    cell = (const __be32 *)prop->data;
>>      device_tree_get_reg(&cell, address_cells, size_cells,
>>                          &mod->start, &mod->size);
>>  
> 
> 


-- 
Julien Grall