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

[Karol Lewandowski <k.lewandowsk@samsung.com>]

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?


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!


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

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