Home
Reading
Searching
Subscribe
Sponsors
Statistics
Posting
Contact
Spam
Lists
Links
About
Hosting
Filtering
Features Download
Marketing
Archives
FAQ
Blog
 
Gmane
From: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ <at> public.gmane.org>
Subject: Re: [PATCH v3 2/2] drivers: dma-contiguous: add initialization from device tree
Newsgroups: gmane.linux.drivers.devicetree
Date: Thursday 11th July 2013 14:56:41 UTC (over 4 years ago)
Hi Marek,

Thanks for working on this. Comments below...

On Wed, Jun 26, 2013 at 2:40 PM, Marek Szyprowski
 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

> Acked-by: Kyungmin Park

> ---
>  .../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:
> [email protected]', [email protected]', [email protected]', ...

Drop the use of 'region'. "[email protected]" 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 [email protected], [email protected] and
> [email protected] 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: [email protected] {
> +                               reg = <0x0 0x4000000>;
> +                               linux,default-contiguous-region;
> +                       };
> +
> +                       /*
> +                        * special region for framebuffer and
> +                        * multimedia processing devices
> +                        */
> +                       display_mem: [email protected] {
> +                               reg = <0x78000000 0x4000000>;
> +                               device = <&fb0 &scaller &codec>;
> +                       };
> +               };
> +       };
> +
> +       /* ... */
> +
> +       fb0: [email protected] {
> +               status = "okay";
> +       };
> +       scaller: [email protected] {
> +               status = "okay";
> +       };
> +       codec: [email protected] {
> +               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 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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
>
 
CD: 3ms