Hello Grant,

On 7/11/2013 4:56 PM, Grant Likely wrote:

Hi Marek,

Thanks for working on this. Comments below...

On Wed, Jun 26, 2013 at 2:40 PM, Marek Szyprowski
[off-list ref] wrote:

quoted

Add device tree support for contiguous memory regions defined in device
tree. Initialization is done in 2 steps. First, the contiguous memory is
reserved, what happens very early when only flattened device tree is
available. Then on device initialization the corresponding cma regions are
assigned to each device structure.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/contiguous-memory.txt      |   94 ++++++++++++++
 arch/arm/boot/dts/skeleton.dtsi                    |    7 +-
 drivers/base/dma-contiguous.c                      |  132 ++++++++++++++++++++
 3 files changed, 232 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/contiguous-memory.txt

diff --git a/Documentation/devicetree/bindings/contiguous-memory.txt b/Documentation/devicetree/bindings/contiguous-memory.txt
new file mode 100644
index 0000000..a733df2
--- /dev/null
+++ b/Documentation/devicetree/bindings/contiguous-memory.txt

@@ -0,0 +1,94 @@
+*** Contiguous Memory binding ***
+
+The /chosen/contiguous-memory node provides runtime configuration of
+contiguous memory regions for Linux kernel. Such regions can be created
+for special usage by various device drivers. A good example are
+contiguous memory allocations or memory sharing with other operating
+system(s) on the same hardware board. Those special memory regions might
+depend on the selected board configuration and devices used on the target
+system.
+
+Parameters for each memory region can be encoded into the device tree
+with the following convention:
+
+contiguous-memory {
+
+       (name): region@(base-address) {
+               reg = <(baseaddr) (size)>;
+               (linux,default-contiguous-region);
+               device = <&device_0 &device_1 ...>
+       };
+};

Okay, I've gone and read all of the backlog on the 3 versions of the
patch series, and I think I understand the issues. I actually think it
was better off to have the regions specified as children of the memory
node. I understand the argument about how would firmware know what
size the kernel wants and that it would be better to have a kernel
parameter to override the default. However, it is also reasonable for
the kernel to be provided with a default amount of CMA based on the
usage profile of the device. In that regard it is absolutely
appropriate to put the CMA region data into the memory node. I don't
think /chosen is the right place for that.

Thanks for your comments! I will prepare a new version, which will use
/memory node as it was in the first version. I hope that with Your ack
such version can be finally merged.

quoted

+
+name:          an name given to the defined region;

In the above node example, "name:" is a label used for creating
phandles. That information doesn't appear in the resulting .dtb
output. The label is actually optional It should instead be:
         [(label):] (name)@(address) { }

quoted

+base-address:  the base address of the defined region;
+size:          the size of the memory region (bytes);

The reg property should follow the normal reg rules which are well
defined. That also means that a memory region could have multiple reg
entries if appropriate.

Well, I'm not sure if it really makes sense to support multiple reg 
entries.
I also wonder how to provide entries for LPAE systems. They are 32-bit 
systems,
but physical addresses can be up to 36bit. How to specify them in device 
tree?

quoted

+linux,default-contiguous-region: property indicating that the region
+               is the default region for all contiguous memory
+               allocations, Linux specific (optional);
+device:                array of phandles to the client device nodes, which
+               will use the defined contiguous region.

This is backwards compared to the way device references usually work.
It would be more consistent for each device node to have a
"dma-memory-region" property with phandles to one or more memory
regions that it cares about.

quoted

+Each defined region must use unique name. It is optional to specify the
+base address, so if one wants to use autoconfiguration of the base
+address, he must specify the '0' as base address in the 'reg' property
+and assign ann uniqe name to such regions, following the convention:
+'region at 0', 'region at 1', 'region at 2', ...

Drop the use of 'region'. "name at 0" is more typical. It would be
appropriate to have compatible = "reserved-memory-region" in each of
the reserved regions. That would avoid the problem of two regions
based at the same address having a conflict in name.

Ok.

 > ...

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland