[RFC/PATCH v2 2/2] drivers: dma-contiguous: add initialization from device tree

[Marek Szyprowski]

Hi Laura,

Thanks for your thorough review! I will fix all the pointed issues once the
main point of this patch set (using /chosen/contiguous-memory for CMA DT
bindings) will be agreed and accepted.

On 4/11/2013 7:56 PM, Laura Abbott wrote:
> Hi,
>
> On 4/11/2013 4:22 AM, Marek Szyprowski wrote:
> ...
>> +
>> diff --git a/drivers/base/dma-contiguous.c 
>> b/drivers/base/dma-contiguous.c
>> index 01fe743..6a8abab 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,53 @@ 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;
>> +
>> +    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;
>> +
>
> Requiring the region@ label does not work if you want two dynamically 
> placed regions (i.e. two region at 0). The devicetree will take the last 
> region at 0 entry and ignore all the other ones


Hmm. You are right, I need different solution here to get it working 
with autoconfigured base address.

>> +    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);
>> +
>> +    dma_contiguous_reserve_area(size, base, 0, &cma);
>> +
>
> Need to check the return of dma_contiguous_reserve_area, else there is 
> an abort when trying to access cma->name on an error
>
>> +    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).
>
>
> if the contiguous region isn't set via devicetree, 
> default_area->full_name needs to be set in dma_contiguous_reserve, 
> else we get wrong associations in scan_cma_node.
>
>> @@ -149,6 +200,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 +320,71 @@ int __init dma_contiguous_add_device(struct 
>> device *dev, struct cma *cma)
>>       return 0;
>>   }
>>
>> +#ifdef CONFIG_OF
>> +static struct cma_map {
>> +    struct cma *cma;
>> +    struct device_node *node;
>> +} cma_maps[MAX_CMA_AREAS];
>> +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++)
>> +            if (strstr(child->full_name, cma_areas[i].full_name))
>> +                cma = &cma_areas[i];
>
> break out of the loop once the area is found?
>
> Also, how will the code deal with region names that are substrings of 
> each other e.g.
>
> region at 1000000
> region at 10000000
>

Thanks for pointing this.

>> +        if (!cma)
>> +            continue;
>> +
>> +        for (i=0;; i++) {
>> +            struct device_node *node;
>> +            node = of_parse_phandle(child, "device", i);
>> +            if (!node)
>> +                break;
>> +
>> +            cma_maps[cma_map_count].cma = cma;
>> +            cma_maps[cma_map_count].node = node;
>> +            cma_map_count++;
>
> Bounds check cma_map_count against MAX_CMA_AREAS?
>
>> +        }
>> +    }
>> +}
>> +#endif
>> +
>>   static int __init cma_init_reserved_areas(void)
>>   {
>>       int i;
>> @@ -275,6 +395,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);
>>
>
> Thanks,
> Laura
>

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