Re: [PATCH] linuxppc/devtree: Parse new DRC mem/cpu/dev device tree elements

[Michael Bringmann <mwb@linux.vnet.ibm.com>]

Responses to your remarks about the patch.  Note that I will repost it in
smaller segments later this week.

On 07/13/2016 03:41 PM, Nathan Fontenot wrote:
> On 06/30/2016 04:44 PM, Michael Bringmann wrote:
>> Several properties in the DRC device tree format are replaced by
>> more compact representations to allow, for example, for the encoding
>> of vast amounts of memory, and or reduced duplication of information
>> in related data structures.
>>
>> "ibm,drc-info": This property, when present, replaces the following
>> four properties: "ibm,drc-indexes", "ibm,drc-names", "ibm,drc-types"
>> and "ibm,drc-power-domains".  This property is defined for all
>> dynamically reconfigurable platform nodes.  The "ibm,drc-info" elements
>> are intended to provide a more compact representation, and reduce some
>> search overhead.
>>
>> "ibm,dynamic-memory-v2": This property replaces the "ibm,dynamic-memory"
>> node representation within the "ibm,dynamic-reconfiguration-memory"
>> property provided by the BMC.  This element format is intended to provide
> 
> BMC?

Just a term for the underlying platform.  I think that it came from a
conversation with another developer.  We can just use 'underlying platform'.

>> +#define	DRCONF_V2_CELL_OFFSET(i)	((i) * DRCONF_V2_CELLS_LEN)
>> +#define	DRCONF_V2_CELL_POSITION(p, i)	\
>> +			(void *)(((char *)(p))+((i) * DRCONF_V2_CELLS_LEN))
>> +#define DYN_MEM_V2_LEN(entries)	(((entries) * DRCONF_V2_CELLS_LEN) + \
>> +				 (1 * sizeof(unsigned int)))
>> +
> 
> These should probably be functions instead of #defines, makes debugging
> the code easier.

6-of-1 or half-a-dozen to me.  The main reason that I made them macros was
to document the size calculation in one place, instead of having it embedded
in multiple locations in the code as was done for the 'ibm,dynamic-memory'
struct parsing.

> 
>> +#define DRCONF_MEM_PRESERVED		0x00000001
>> +#define DRCONF_MEM_PRESERVABLE		0x00000002
>> +#define DRCONF_MEM_PRESERVED_STATE	0x00000004
>> +#define DRCONF_MEM_ASSIGNED		0x00000008
>> +#define DRCONF_MEM_NO_H_MIGRATE_DATA	0x00000010
>> +#define DRCONF_MEM_DRC_INVALID		0x00000020
>> +#define DRCONF_MEM_AI_INVALID		0x00000040
>> +#define DRCONF_MEM_RESERVED		0x00000080
>> +#define DRCONF_MEM_RESERVED_SW		0x80000000
> 
> I'll let others chime in, but we don't use all of these flags, or plan
> to at this point so I'm not sure we need to include definitions for them.

I can cut down the list.  3 were previously defined in this file.  
> 

>>  /*
>> - * Retrieve and validate the ibm,dynamic-memory property of the device tree.
>> + * Read the next memblock set entry from the ibm,dynamic-memory-v2 property
> 
> Just saw this here, ans see that it is used elsewhere. You may want to avoid
> using the term memblock, this already has a meaning in the kernel and may
> cause some confusion.
> 
> Still reviewing this patch, more comments as I review more.
> 
> -Nathan

'memblock' was used by the original comments for 'ibm,dynamic-memory' structures.
I will change them.

> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com