Re: [PATCH v6 2/4] drivers: of: add function to scan fdt nodes given by path

Re: [PATCH v6 2/4] drivers: of: add function to scan fdt nodes given by path

[Rob Herring]

On Mon, Aug 19, 2013 at 10:04 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Add a function to scan the flattened device-tree starting from the
> node given by the path. It is used to extract information (like reserved
> memory), which is required on early boot before we can unflatten the tree.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Acked-by: Tomasz Figa <t.figa@samsung.com>

Reviewed-by: Rob Herring <rob.herring@calxeda.com>

Re: [PATCH v6 3/4] drivers: of: add initialization code for dma reserved memory

[Rob Herring]

On Mon, Aug 19, 2013 at 10:04 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> This patch adds device tree support for contiguous and reserved memory
> regions defined in device tree.
>
> Large memory blocks can be reliably reserved only during early boot.
> This must happen before the whole memory management subsystem is
> initialized, because we need to ensure that the given contiguous blocks
> are not yet allocated by kernel. Also it must happen before kernel
> mappings for the whole low memory are created, to ensure that there will
> be no mappings (for reserved blocks) or mapping with special properties
> can be created (for CMA blocks). This all happens before device tree
> structures are unflattened, so we need to get reserved memory layout
> directly from fdt.
>
> Later, those reserved memory regions are assigned to devices on each
> device structure initialization.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Acked-by: Tomasz Figa <t.figa@samsung.com>

Reviewed-by: Rob Herring <rob.herring@calxeda.com>

Re: [PATCH v6 3/4] drivers: of: add initialization code for dma reserved memory

[Grant Likely]

On Tue, 20 Aug 2013 00:02:37 +0200, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Stephen,
> 
> On Monday 19 of August 2013 15:49:20 Stephen Warren wrote:
> > On 08/19/2013 09:04 AM, Marek Szyprowski wrote:
> > > This patch adds device tree support for contiguous and reserved memory
> > > regions defined in device tree.
> > > 
> > > diff --git a/Documentation/devicetree/bindings/memory.txt
> > > b/Documentation/devicetree/bindings/memory.txt
> > > 
> > > +*** Reserved memory regions ***
> > > +
> > > +In /memory/reserved-memory node one can create additional nodes
> > 
> > s/additional/child/ or s/additional/sub/ would make it clearer where the
> > "additional" nodes should be placed.
> > 
> > > +compatible:	"linux,contiguous-memory-region" - enables binding 
> of
> > > this
> > > +		region to Contiguous Memory Allocator (special region for
> > > +		contiguous memory allocations, shared with movable system
> > > +		memory, Linux kernel-specific), alternatively if
> > > +		"reserved-memory-region" - compatibility is defined, given
> > > +		region is assigned for exclusive usage for by the 
> respective
> > > +		devices
> > 
> > "alternatively" makes it sound like the two compatible values are
> > mutually-exclusive. Perhaps make this a list, like:
> > 
> > ----------
> > compatible: One or more of:
> > 
> > 	- "linux,contiguous-memory-region" - enables binding of this
> > 	  region to Contiguous Memory Allocator (special region for
> > 	  contiguous memory allocations, shared with movable system
> > 	  memory, Linux kernel-specific).
> > 	- "reserved-memory-region" - compatibility is defined, given
> > 	  region is assigned for exclusive usage for by the respective
> > 	  devices.
> > ----------
> > 
> > "linux,contiguous-memory-region" is already long enough, but I'd
> > slightly bikeshed towards "linux,contiguous-memory-allocator-region", or
> > perhaps "linux,cma-region" since it's not really describing whether the
> > memory is contiguous (at the level of /memory, each chunk of memory is
> > contiguous...)
> 
> I'm not really sure if we need the "linux" prefix for "contiguous-memory-
> region". The concept of contiguous memory region is rather OS independent 
> and tells us that memory allocated from this region will be contiguous. 
> IMHO any OS is free to implement its own contiguous memory allocation 
> method, without being limited to Linux CMA.
> 
> Keep in mind that rationale behind those contiguous regions was that there 
> are devices that require buffers contiguous in memory to operate 
> correctly.
> 
> But this is just nitpicking and I don't really have any strong opinion on 
> this.

Actually, I think this is important. It is something that describes the
expected usage of the hardware (flicker-free framebuffer is a really
good example) and isn't really linux-specific from that perspective.
I think you can drop the 'linux,' prefix string.

g.

Re: [PATCH v6 3/4] drivers: of: add initialization code for dma reserved memory

[Grant Likely]

On Wed, 21 Aug 2013 17:39:48 +0200, Tomasz Figa <t.figa@samsung.com> wrote:
> On Tuesday 20 of August 2013 10:35:51 Stephen Warren wrote:
> > >> a) Adjust memory.txt to allow multiple entries in memory-regions, and
> > >> add an associated memory-region-names property.
> > >> 
> > I don't believe that's a good approach unless you have at least a
> > partial idea of how the current bindings will be extended to support
> > multiple memory regions.
> 
> I believe that at least three "at least partial" ideas have been brought in 
> this thread. Moreover, I don't think that defining the simple binding now 
> would stop us from extending it according to any of those ideas in any way.

I would plan on extending later with suggestion 'a)' above. Making the
memory-region an array of phandles is a better match with how most of
the core bindings work.  I actually regret the *-gpio properties that
were defined earlier becasue it makes it harder for software to look at
a tree and determine if there are any gpio references.

It of course doesn't need to be done immediately, only when it becomes
required.

g.