Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

On 08/28/2012 07:08 PM, Arun Kumar K wrote:

Hi Karol,
Thanks for your comments. 
Please find my response inline.

Hi... and sorry for so much delayed response.

quoted

quoted

+
+static void __init exynos5_reserve(void)
+{
+     s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);

I think it would make sense to make this memory reservation dependent
on "mfc*" node being present in DTS.  It's to early to use of_* functions
(because tree is not populated at this stage) but fdt_* family of functions
work just fine.

As I can see the fdt_* functions are not used in any of the ARM based SoC
init codes. Though I can see some references in powerpc.
The implementation and includes are present in arch/arm/boot/compressed/
which I think cannot be used directly in mach-exynos unless we make some
comon makefile changes. 

It looks like I was writing from memory, and I actually mixed things 
up.  To be clear this time - we can't use regular device tree handling
functions in reserve() as it's too early.  Namely, flattened device tree
is not yet converted to kernel's-natural representation.  However, 
I think we can scan fdt just fine.  To do so one just needs to use
functions declared here

  #include <linux/of_fdt.h>

Actual architecture-independent code is in drivers/of/fdt.c.  This
provides of_fdt_ family of functions. Please see below for example.

Please clarify whether its ok to use fdt_* functions to parse the dts in 
exynos machine init or please point me to some sample implementations
which I can refer to.

It should be ok to use anything that works on flattened device tree
rather than its uncompressed version.  I've experimented a bit and
something like this worked for me just fine (it was around 3.3-kernel
timeframe, but I don't think that fdt api has changed):

[mach-exynos4-dt.c]

#include <linux/of_fdt.h>

int fdt_find_compat(unsigned long node, const char *uname, int depth, void *data)
{
        if (of_flat_dt_is_compatible(node, (char *)data))
                return 1;

        return 0;
}

static void __init exynos4210_dt_reserve(void)
{
        /* Reserve memory for MFC only if it's available */
        if (of_scan_flat_dt(fdt_find_compat, "samsung,s5pv210-mfc")) {
                printk(KERN_NOTICE "exynos4-dt: mfc device node found - setting up memory area for dma\n");
                s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
        }
}

[.dts]

 codec@some-addr {
      compatible = "samsung,s5pv210-mfc";
 };


So, in above code fragment I just check if mfc was defined in dts.
This could probably stay as it is.

Then I allocate _predefined_ region - and this part should be fixed.

If you have nodes like "mfc-r-size"/offset, then you could just get
this information directly from (f)dt rather than hardcoding it in the code.
Precisely, after we find compatible node we could do something like
following (untested):

unsigned long lsize, loff, rsize, roff len;
__be32 *prop;

prop = of_get_flat_dt_prop(node, "samsung,mfc-l-size", &len);
if (!prop)
   return;
lsize = of_read_ulong(prop, len/4);
...

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform