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

[Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>]

On 10/17/2013 03:54 PM, David Daney wrote:
> 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

Yes, I forgot to use the generic variables for vpe.o and rtlx.o in my last 
reply. Your solution above is right what I wanted to discuss. Thanks for 
the catch of large ifdefs in the C files.


Deng-Cheng