Hi Marek, Thanks for working on this. Comments below...
On Wed, Jun 26, 2013 at 2:40 PM, Marek Szyprowski <m.szyprow...@samsung.com> wrote: > 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.szyprow...@samsung.com> > Acked-by: Kyungmin Park <kyungmin.p...@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. > + > +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) { } > +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. > +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. > +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@0', 'region@1', 'region@2', ... Drop the use of 'region'. "name@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. > + > + > +*** Example *** > + > +This example defines a memory configuration containing 2 contiguous > +regions for Linux kernel, one default of all device drivers (named > +contig_mem, autoconfigured at boot time, 64MiB) and one dedicated to the > +framebuffer device (named display_mem, placed at 0x78000000, 64MiB). The > +display_mem region is then assigned to fb@12300000, scaller@12500000 and > +codec@12600000 devices for contiguous memory allocation with Linux > +kernel drivers. > + > +The reason for creating a separate region for framebuffer device is to > +match the framebuffer base address to the one configured by bootloader, > +so once Linux kernel drivers starts no glitches on the displayed boot > +logo appears. Scaller and codec drivers should share the memory > +allocations with framebuffer driver. > + > +/ { > + /* ... */ > + > + chosen { > + bootargs = /* ... */ > + > + contiguous-memory { > + > + /* > + * global autoconfigured region > + * for contiguous allocations > + */ > + contig_mem: region@0 { > + reg = <0x0 0x4000000>; > + linux,default-contiguous-region; > + }; > + > + /* > + * special region for framebuffer and > + * multimedia processing devices > + */ > + display_mem: region@78000000 { > + reg = <0x78000000 0x4000000>; > + device = <&fb0 &scaller &codec>; > + }; > + }; > + }; > + > + /* ... */ > + > + fb0: fb@12300000 { > + status = "okay"; > + }; > + scaller: scaller@12500000 { > + status = "okay"; > + }; > + codec: codec@12600000 { > + status = "okay"; > + }; > +}; > diff --git a/arch/arm/boot/dts/skeleton.dtsi b/arch/arm/boot/dts/skeleton.dtsi > index b41d241..cadc3b9 100644 > --- a/arch/arm/boot/dts/skeleton.dtsi > +++ b/arch/arm/boot/dts/skeleton.dtsi > @@ -7,7 +7,12 @@ > / { > #address-cells = <1>; > #size-cells = <1>; > - chosen { }; > + chosen { > + contiguous-memory { > + #address-cells = <1>; > + #size-cells = <1>; > + }; > + }; > aliases { }; > memory { device_type = "memory"; reg = <0 0>; }; > }; > diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c > index 01fe743..ce5f5d1 100644 > --- a/drivers/base/dma-contiguous.c > +++ b/drivers/base/dma-contiguous.c > @@ -24,6 +24,9 @@ > > #include <linux/memblock.h> > #include <linux/err.h> > +#include <linux/of.h> > +#include <linux/of_fdt.h> > +#include <linux/of_platform.h> > #include <linux/mm.h> > #include <linux/mutex.h> > #include <linux/page-isolation.h> > @@ -37,6 +40,7 @@ struct cma { > unsigned long base_pfn; > unsigned long count; > unsigned long *bitmap; > + char full_name[32]; > }; > > static DEFINE_MUTEX(cma_mutex); > @@ -133,6 +137,52 @@ static __init int cma_activate_area(struct cma *cma) > return 0; > } > > +/*****************************************************************************/ > + > +#ifdef CONFIG_OF > +int __init cma_fdt_scan(unsigned long node, const char *uname, > + int depth, void *data) > +{ > + static int level; > + phys_addr_t base, size; > + unsigned long len; > + struct cma *cma; > + __be32 *prop; > + int ret; > + > + if (depth == 1 && strcmp(uname, "chosen") == 0) { > + level = depth; > + return 0; > + } > + > + if (depth == 2 && strcmp(uname, "contiguous-memory") == 0) { > + level = depth; > + return 0; > + } > + > + if (level != 2 || depth != 3 || strncmp(uname, "region@", 7) != 0) > + return 0; > + > + prop = of_get_flat_dt_prop(node, "reg", &len); > + if (!prop || (len != 2 * sizeof(unsigned long))) > + return 0; > + > + base = be32_to_cpu(prop[0]); > + size = be32_to_cpu(prop[1]); > + > + pr_info("Found %s, memory base %lx, size %ld MiB\n", uname, > + (unsigned long)base, (unsigned long)size / SZ_1M); > + > + ret = dma_contiguous_reserve_area(size, base, 0, &cma); > + if (ret == 0) { > + strcpy(cma->full_name, uname); > + if (of_get_flat_dt_prop(node, > "linux,default-contiguous-region", NULL)) > + dma_contiguous_default_area = cma; > + } > + return 0; > +} > +#endif > + > /** > * dma_contiguous_reserve() - reserve area(s) for contiguous memory handling > * @limit: End address of the reserved memory (optional, 0 for any). > @@ -149,6 +199,10 @@ void __init dma_contiguous_reserve(phys_addr_t limit) > > pr_debug("%s(limit %08lx)\n", __func__, (unsigned long)limit); > > +#ifdef CONFIG_OF > + of_scan_flat_dt(cma_fdt_scan, NULL); > +#endif > + > if (size_cmdline != -1) { > sel_size = size_cmdline; > } else { > @@ -265,6 +319,80 @@ int __init dma_contiguous_add_device(struct device *dev, > struct cma *cma) > return 0; > } > > +#ifdef CONFIG_OF > + > +#define MAX_CMA_MAPS 64 > + > +static struct cma_map { > + struct cma *cma; > + struct device_node *node; > +} cma_maps[MAX_CMA_MAPS]; > +static unsigned cma_map_count; > + > +static void cma_assign_device_from_dt(struct device *dev) > +{ > + int i; > + for (i = 0; i < cma_map_count; i++) { > + if (cma_maps[i].node == dev->of_node) { > + dev_set_cma_area(dev, cma_maps[i].cma); > + pr_info("Assigned CMA %s to %s device\n", > + cma_maps[i].cma->full_name, > + dev_name(dev)); > + } > + } > +} > + > +static int cma_device_init_notifier_call(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct device *dev = data; > + if (event == BUS_NOTIFY_ADD_DEVICE && dev->of_node) > + cma_assign_device_from_dt(dev); > + return NOTIFY_DONE; > +} > + > +static struct notifier_block cma_dev_init_nb = { > + .notifier_call = cma_device_init_notifier_call, > +}; > + > +void scan_cma_nodes(void) > +{ > + struct device_node *parent = > of_find_node_by_path("/chosen/contiguous-memory"); > + struct device_node *child; > + > + if (!parent) > + return; > + > + for_each_child_of_node(parent, child) { > + struct cma *cma = NULL; > + int i; > + > + for (i = 0; i < cma_area_count; i++) { > + char *p = strrchr(child->full_name, '/') + 1; > + if (strcmp(p, cma_areas[i].full_name) == 0) > + cma = &cma_areas[i]; > + } > + if (!cma) > + continue; > + > + for (i = 0;; i++) { > + struct device_node *node; > + node = of_parse_phandle(child, "device", i); > + if (!node) > + break; > + > + if (cma_map_count < MAX_CMA_MAPS) { > + cma_maps[cma_map_count].cma = cma; > + cma_maps[cma_map_count].node = node; > + cma_map_count++; > + } else { > + pr_err("CMA error: too many devices > defined\n"); > + } > + } > + } > +} > +#endif > + > static int __init cma_init_reserved_areas(void) > { > int i; > @@ -275,6 +403,10 @@ static int __init cma_init_reserved_areas(void) > return ret; > } > > +#ifdef CONFIG_OF > + scan_cma_nodes(); > + bus_register_notifier(&platform_bus_type, &cma_dev_init_nb); > +#endif > return 0; > } > core_initcall(cma_init_reserved_areas); > -- > 1.7.9.5 > _______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss