Re: [PATCH 2/6] MIPS: APRP: Add VPE loader support for CMP platforms.

[David Daney <ddaney.cavm@gmail.com>]

On 10/17/2013 03:38 PM, Deng-Cheng Zhu wrote:
> On 10/17/2013 03:11 PM, David Daney wrote:
>> On 10/17/2013 03:00 PM, Deng-Cheng Zhu wrote:
>>> On 10/17/2013 10:40 AM, David Daney wrote:
>>>> On 10/16/2013 07:14 PM, Steven J. Hill wrote:
>>>>> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>>>>
>>>>> This patch adds VPE loader support for platforms having a CMP.
>>>>>
>>>>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>>>> Signed-off-by: Steven J. Hill <Steven.Hill@imgtec.com>
>>>>> Reviewed-by: Qais Yousef <Qais.Yousef@imgtec.com>
>>>>> ---
>>>>>   arch/mips/kernel/Makefile  |    2 +-
>>>>>   arch/mips/kernel/vpe-cmp.c |  184
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>   arch/mips/kernel/vpe-mt.c  |    4 +
>>>>>   3 files changed, 189 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 arch/mips/kernel/vpe-cmp.c
>>>>>
>>>>> diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
>>>>> index 51f9117..912eb64 100644
>>>>> --- a/arch/mips/kernel/Makefile
>>>>> +++ b/arch/mips/kernel/Makefile
>>>>> @@ -54,7 +54,7 @@ obj-$(CONFIG_MIPS_MT_SMP)    += smp-mt.o
>>>>>   obj-$(CONFIG_MIPS_CMP)        += smp-cmp.o
>>>>>   obj-$(CONFIG_CPU_MIPSR2)    += spram.o
>>>>>
>>>>> -obj-$(CONFIG_MIPS_VPE_LOADER)    += vpe.o vpe-mt.o
>>>>> +obj-$(CONFIG_MIPS_VPE_LOADER)    += vpe.o vpe-cmp.o vpe-mt.o
>>>>>   obj-$(CONFIG_MIPS_VPE_APSP_API) += rtlx.o
>>>>>
>>>>>   obj-$(CONFIG_I8259)        += i8259.o
>>>>> diff --git a/arch/mips/kernel/vpe-cmp.c b/arch/mips/kernel/vpe-cmp.c
>>>>> new file mode 100644
>>>>> index 0000000..a5628ca
>>>>> --- /dev/null
>>>>> +++ b/arch/mips/kernel/vpe-cmp.c
>>>>> @@ -0,0 +1,184 @@
>>>>> +/*
>>>>> + * This file is subject to the terms and conditions of the GNU
>>>>> General Public
>>>>> + * License.  See the file "COPYING" in the main directory of this
>>>>> archive
>>>>> + * for more details.
>>>>> + *
>>>>> + * Copyright (C) 2004, 2005 MIPS Technologies, Inc.  All rights
>>>>> reserved.
>>>>> + * Copyright (C) 2013 Imagination Technologies Ltd.
>>>>> + */
>>>>> +#ifdef CONFIG_MIPS_CMP
>>>>> +
>>>>
>>>> Get rid of all these #ifdef.
>>>>
>>>> Use Kconfig symbols in the makefile instead.
>>>>
>>>>
>>>
>>> Right. Splitting stuff into -cmp/-mt files is an effort to remove such
>>> kind of #ifdef. The example can be found in Makefile in the v4 of this
>>> patch set: http://patchwork.linux-mips.org/patch/5059/
>>>
>>>
>>
>> OK, that patch you point to seems a little better, but there are still
>> ifdefs in the Makefile.  You can create synthetic Kconfig variables so
>> the makefile is cleaner, but I don't know if it is worth it in this case.
>
> Hmm. That has pros and cons, IMO. So the Makefile will look like:
>
> obj-$(CONFIG_MIPS_VPE_LOADER_CMP)  += vpe.o vpe-cmp.o
> obj-$(CONFIG_MIPS_VPE_APSP_API_CMP) += rtlx.o rtlx-cmp.o
> obj-$(CONFIG_MIPS_VPE_LOADER_MT)  += vpe.o vpe-mt.o
> obj-$(CONFIG_MIPS_VPE_APSP_API_MT) += rtlx.o rtlx-mt.o
>
> It removes ifdef, but doesn't look straightforward to me: CMP and MT
> APRP are mutually exclusive.

It is not necessarily cleaner but you could have something like:
---------------------
config MIPS_VPE_LOADER_CMP
	bool
	default "y"
	depends on MIPS_VPE_LOADER && MIPS_CMP

config MIPS_VPE_LOADER_MT
	bool
	default "y"
	depends on MIPS_VPE_LOADER && !MIPS_CMP

----------
obj-$(CONFIG_MIPS_VPE_LOADER)	+= vpe.o
obj-$(CONFIG_MIPS_VPE_LOADER_CMP)	+= vpe-cmt.o
obj-$(CONFIG_MIPS_VPE_LOADER_MT)	+= vpe-mt.o


I would do either that, or what you have in 
http://patchwork.linux-mips.org/patch/5059/  either is acceptable I think.

My main objection was the thing about putting the #ifdefs around the 
entire body of the C files.

David Daney