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

[Arun Kumar K <arun.kk@samsung.com>]

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

On Mon, Aug 20, 2012 at 11:47 AM, Karol Lewandowski <k.lewandowsk@samsung.com> wrote:
> On 08/16/2012 08:42 PM, Thomas Abraham wrote:
>> On 16 August 2012 18:01, Arun Kumar K <arun.kk ... @public.gmane.org> wrote:
>
>>> +  - 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:
>
>> +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.

>
>> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
>
>> index ef770bc..898d2de 100644
>> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
>> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
> ...
>> +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.
>
>> +
>> +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