[PATCH 0/5] arm/tms570: include hardware initialization and selftests to run RTEMS directly from Flash without loader on TMS570LS3137 HDK.

[Gedare Bloom]

On Tue, Sep 13, 2016 at 5:11 PM, Pavel Pisa <pisa at cmp.felk.cvut.cz> wrote:
> Hello Gedare,
>
> thanks for the review. It is huge piece and not in a state
> as nice as I would like it.
>
> On Tuesday 13 of September 2016 21:42:48 Gedare Bloom wrote:
>> I'm slowly reading through these. Is there a doc or is it worth it to
>> generate one that maps the HalcoGen functions to their RTEMS renamed
>> versions? I'm not sure if it is of much value.
>
> I am not sure at the end. I have tried to change names
> to RTEMS, Linux, POSIX style. The rest of BSP (which
> is written mostly from scratch) already follows that.
> Other option is to use Ti CammelCase and add tms570_ prefix.
> Genrally I am not sure. I have not renamed functions
> in the ARM core support tms570_sys_core.S .
> But if we agree that all should follow single naming
> convention I take a while for that.
> But there could be value in preserving Ti names either.
>
I wouldn't mind simple tms570_preserveCamelCase() as an easy rule to
follow and to track back clearly to what came from halcogen versus own
code that should use bsp style (tms570_xxx_yyy_zzz).

>> I'm slowly going through this big one. One quick note I wanted to make
>> is to ask you to make comments where you have used #if 0 ... #endif to
>> comment out code.
>
> Yes, I should comment that more.
> Some small parts are only comments by code sections.
> But more text would help there.
>
> But the biggest commented out blocks are in bspstarthooks-hwinit.c.
> I probably try to move this code to subroutines.
> The two bigger parts are internal main SRAM selftest
> and test of reaction to SRAM content protection against bitflip
> errors.
>
> I plan to try to reenable at least selftest for cases where
> it does not lead to crash. But these testes has to be skipped
> for case when RTEMS application is linked and run from SRAM
> and even for case when SRAM is used as overlay for FLASH
> when RTEMS application is not starting from address zero.
> Or at least I have problems with these cases.
> When we have already finally link mode where RTEMS
> can be the first (and only) Flash content starting
> directly then for this mode code should work.
> But requires more experiments.
>
> Masks for tms570_pbist_run should be converted to
> some defines as well -> to my TODO.
>
> Complete RAM ECC reporting checking then triggers
> real uncorrectable error fault which leads to ARM level
> exception which is catch and only if that all happens
> then selftest continues, else fault is reported.
> All that requires that code runs from Flash and does
> not touch RAM except for precise triggering events.
> So again quite huge task and requiring modification
> of RTEMS ARM exception table.
>
>> Is "partest" halcogen term, or you chose it? I might prefer
>> "parity_check" instead (and remove any redundant "parity_check_check".
>
> I looked for some shorthand to have sane length names.
> But I agree that they could be misleading. May it be parck_
> to be short? On the other hand parity_check_ is more clear.
> But in the fact it is "selftest of parity check reporting
> mechanism health".
>
maybe for this selftest code it should all be prefixed with selftest_,
then this is selftest_par_xxx? Makes it a bit more clear when casual
reading I guess.

I'm still making slow progress.

> Best wishes,
>
>                Pavel
>
>