Re: [PATCH v2 1/2] MIPS: fix/enrich 34K APRP (APSP) functionalities

[Deng-Cheng Zhu <dczhu@mips.com>]

Thanks for your detailed explanation! Please see my comments below.

On 05/23/2012 06:12 AM, Maciej W. Rozycki wrote:
> On Tue, 22 May 2012, Deng-Cheng Zhu wrote:
>
>>>    Hmm, there's nothing platform-specific there, the file is pretty generic,
>>> it could be moved to arch/mips/kernel/ or thereabouts.  That applies to
>>> <asm/mips-boards/launch.h>   too, before you ask
>>
>> Yeah, agree with you. I didn't do it simply because I'm not sure :)
>
>   I can see you've copied the whole contents over to arch/mips/kernel/vpe.c
> now.  Please don't do that.  This code is modular for a reason.  Either
> modify original code to suit your needs while preserving functionality or,
> if infeasible, copy it over to a new module selected based on
> configuration.  Common parts should be abstracted and extracted to a
> common dependency, either a shared header or another module, as
> applicable.

OK. Good advice!

>>> (you may want to use alloc_bootmem or suchlike instead of hardcoding the
>>> trampoline page, though it's probably pretty safe to assume the end of
>>> the exception handler page is available everywhere).
>>
>> I'm not quite clear about this. Do you mean to bypass the arbitrary monitor
>> in vpe_run() (in other words, to directly bring up the vpe in vpe_run())?
>> Why do we need to worry about writing to the cpulaunch data?
>
>   The location of RAM is platform-specific, CKSEG0ADDR mustn't be used to
> "allocate" kernel memory.  But as I say the exception handlers' page is
> generally pretty safe, although the addition of the CP0 EBase register to
> the architecture stopped it being guaranteed at one point.
>
>   Ultimately I think this memory should be properly allocated, but this is
> preexisting code, so there is no requirement that you fix that on this
> occasion (or at all), unless you'd really like to.

OK. I'll let it be for now.

>>>    There's nothing platform-specific referred to from arch/mips/kernel/vpe.c
>>> AFAICT (and I trust in Beth having got this piece right).  I reckon it
>>> used to work with CONFIG_MIPS_SIM too, though I could imagine the
>>> configuration got neglected a bit as it is somewhat unusual.
>>
>> Oh, When I said IRQ related stuff I meant the interrupt specific changes in
>> rtlx.c (not vpe.c) which correspond to those in malta-int.c. They are
>> there to resolve some issues (Please refer to the code changes and added
>> comments in these 2 files in PATCH #1 and #2.).
>
>   I still can't see anything platform-specific in rtlx.c (also written by
> Beth, BTW) -- it's all software interrupts that are architectural.  What
> pieces of code and comments are you specifically referring to?

I meant some interrupt specific changes in rtlx.c correspond to platform-
specific ones in malta-int.c. You may simply refer to the latter for the
issues I addressed. The changes to malta-int.c led to the platform
dependency and it seems the issues could not be tackled in generic layer.

>   Also in some places you do stuff like:
>
> #ifdef CONFIG_MIPS_CMP
> int foo(void)
> {
> [something...]
> }
> #else
> int foo(void)
> {
> [something else...]
> }
> #endif
>
> Please don't.  Again these pieces should be separate modules selected by
> configuration, e.g. rtlx.c, rtlx-mt.c and rtlx-cmp.c with the former
> holding the common stuff, and the two latters non-CMP- and CMP-specific
> pieces, respectively (assuming that they are mutually exclusive and can't
> be enabled both at a time in a single kernel binary for some reason).

Thanks, good point.

>   It may make sense to move this whole stuff to a subdirectory at one
> point.

Do you mean to move things like vpe.c, kspd.c and rtlx.c (and the proposed
-mt/-cmp variants) into a directory such as arch/mips/kernel/aprp/?


Deng-Cheng