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

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

On Mon, Aug 20, 2012 at 11:47 AM, Karol Lewandowski [off-list ref] wrote:

On 08/16/2012 08:42 PM, Thomas Abraham wrote:

quoted

On 16 August 2012 18:01, Arun Kumar K <arun.kk ... @public.gmane.org> wrote:

quoted

quoted

+  - interrupts : MFC interupt number to the CPU.
+
+  - samsung,mfc-r : Base address of the first memory bank used by MFC
+                   for DMA contiguous memory allocation.
+
+  - samsung,mfc-r-size : Size of the first memory bank.

It is not allowed to pass buffer base address and size from device
tree. Device tree node should describe only the MFC controller
hardware. Any memory management related information should be handled
outside of device tree. This helps the bindings to be reusable across
multiple operating systems.

The question is where elsewhere this should be described as this is strictly
board-dependent option (number and size of RAM banks are important here).

I agree that base addresses are bad, but I'm not aware of any functionality
that would allow driver (actually, its platform dependent part in
exynosN_reserve() function) to enumerate available memory banks and grab
memory chunks from two distinct banks.

My (lack of) knowledge ARM might be to blame here but I simply don't know
how to achieve this. Any suggestions?

As suggested by kgene, I will pass it from the board specific dts file.

On 08/16/2012 09:31 PM, Arun Kumar K wrote:

quoted

+static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
+             phys_addr_t lbase, unsigned int lsize) {
+
+     if (memblock_remove(lbase, lsize)) {
+             pr_err("Failed to reserve bank1 memory for MFC device\n");
+             WARN_ON(1);
+     }
+
+     if (memblock_remove(rbase, rsize)) {
+             pr_err("Failed to reserve bank2 memory for MFC device\n");
+             WARN_ON(1);
+     }
+}

non-static function with the same name is already defined in
arch/arm/plat-samsung/s5p-dev-mfc.c. Please don't duplicate it,
especially that you seem to be trying to do that twice!

Ok, I will use the existing function.

quoted

diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c

quoted

index ef770bc..898d2de 100644

--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c

...

quoted

+static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
+             phys_addr_t lbase, unsigned int lsize) {
+
+     if (memblock_remove(lbase, lsize)) {
+             pr_err("Failed to reserve bank1 memory for MFC device\n");
+             WARN_ON(1);
+     }
+
+     if (memblock_remove(rbase, rsize)) {
+             pr_err("Failed to reserve bank2 memory for MFC device\n");
+             WARN_ON(1);
+     }
+}

See above.

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. 
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.

Regards
Arun

Regards
Arun