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

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:

quoted

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'.

quoted

+#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.

quoted

+#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.  

quoted

 /*
- * 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