Re: OpenRISC eCos port

[Scott Furman <sfurman at rosum dot com>]

Jonathan Lamour wrote:
> Yep, big patches are (unsurprisingly) more difficult to find the time to
> review. I've been looking at this off-and-on for a while now - sorry it's
> taken so long.

Thanks for the review.  Unfortunately, you've caught me while I'm on
holiday.  I'll be returning on May 5th and I probably won't be able to
review patches until then.  I'll try to answer your questions now, though.

> Here's some questions though:
>
> * .text.ram - that isn't used in the orp MLT files, nor the
> CYGIMP_FORCE_INTERRUPT_HANDLING_CODE_IN_RAM option present. Is this right?
>

On another OpenRISC platform I'm experimenting with a code residency scheme
for RAM-starved environments in which most code is stored in ROM and only
performance-sensitive code is located in RAM.  On this platform, all of eCos
runs out of ROM except for interrupt-handling code (and eventually any other
application/eCos code called frequently enough to justify moving it to RAM).

> * Is there definitely no sbss? Nor will there ever be? Just making sure
> it's an architectural property, not just one of the current compiler.

There isn't any sbss section now, nor am I aware of any plans to add one.  I
had sbss-initializing code in vectors.s, but I pulled it out since I thought
it was superfluous.  I can add it back if you think it's important to
future-proof - perhaps a bit paranoid, but relatively inexpensive.

> * Exception interrupt vectors are copied from ROM to 0x100. Is that
> address a strict architectural requirement? I just want to avoid anything
> like the current problem we have with the ARM HAL which assumes RAM starts
> at 0. If it isn't a requirement something like #including
> CYGHWR_MEMORY_LAYOUT_H and using CYGMEM_REGION_RAM+0x100 would be the way.
> But I don't know what OpenRISC actually needs :-).
>

An exception vector beginning at 0x100 is one of two architecture-defined
possibilities.  Additionally, it's possible that someone would choose to
customize their synthesizable OpenRISC processor by changing to a
non-conformant reset vector.  I'll change the reset vector to a
platform-specific symbolic value.  I don't think it should necessarily be
related to CYGMEM_REGION_RAM through some fixed offset, though.

> * The invocation of cyg_hal_exception_handler doesn't look right...
> nothing is saved! Otherwise if you take an exception there's no saved
> register state to point at... vital for the GDB stub at least. This way
> CYGSEM_HAL_ROM_MONITOR will get used as it's meant to as well (since it
> isn't used at all right now ;)).
>

All the exception state *is* stored before cyg_hal_exception_handler() is
called.  (Check out the expansion of the exception_vector assembly macro in
vectors.S. )  I know that this "works" because I have hit gdb exceptions for
illegal instructions, bus errors, etc. and stack backtraces were
sensible-looking.

> * I can't help but think that check_for_external_interrupts should live in
> the platform HAL as the platform has a better idea of the external
> interrupts supported and therefore the relative priorities.

The PIC (interrupt controller) is standardized as part of the OpenRISC
processor (though optional).  That was my motivation for putting support for
the interrupt controller in the arch directory.  On reflection, I agree with
you that the interrupt-handling code should be platform-specific since, as
you say, even a platform that uses PIC might choose different relative
priorities for the peripherals.  I'm not sure that I'll get to splitting out
the interrupt code in the near future, though.

> * You add a comment in hal_openrisc_orp.cdl about CYGNUM_HAL_RTC_PERIOD
> which should be calculated not default_value. Indeed that's what the CDL
> option CYGNUM_KERNEL_COUNTERS_CLOCK_OVERRIDE_PERIOD is for.

Though I don't recall why I wrote the comment, I don't think that
CYGNUM_KERNEL_COUNTERS_CLOCK_OVERRIDE_PERIOD had the same effect.  I don't
have access to the source code now, so I'll have to answer this question
later on.

> * If you're using this with RedBoot do you have a potted .ecm file to go
> with this? And also when we're done, remember to send me some prebuilt
> known good redboot images I can check in to our redboot image repository
> (ecos/images which isn't checked out by default so you won't have seen
it).

I have a Redboot *.ecc file, but not a *.ecm file.  Do these have to be
created by hand or is there some tool/methodology ?

> I've made a number of minor tweaks myself, and I've attached the patches
> for those - almost all trivial as you can see

What's the purpose of the EOF comments ?  Is that just for readability or
does some tool count on them ?

-Scott