This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: PrPMC1100 port
>>>>> Jonathan Larmour writes:
> Hi Mark,
> Perhaps a bit nitpicky, but here's a few comments...
> Mark Salter wrote:
>> + cdl_component CYGPKG_HAL_ARM_XSCALE_PRPMC1100_OPTIONS {
>> + display "Motorola PrPMC1100 build options"
>> + flavor none
>> + no_define
>> + description "
>> + Package specific build options including control over
>> + compiler flags used only in building this package,
>> + and details of which tests are built."
>> +[snip]
>> + cdl_option CYGNUM_HAL_BREAKPOINT_LIST_SIZE {
>> + display "Number of breakpoints supported by the HAL."
>> + flavor data
>> + default_value 32
>> + description "
>> + This option determines the number of breakpoints supported by the HAL."
>> + }
>> + }
> Should CYGNUM_HAL_BREAKPOINT_LIST_SIZE really be in the build options
> component? Looks like the ixdp425 is the same.
Probably not. Looks like all of the XScale and probably a few other ports
are the same way. This should probably get moved into the common hal since
its not ARM specific.
>> + cdl_option CYGSEM_HAL_USE_ROM_MONITOR {
>> + display "Work with a ROM monitor"
>> + flavor booldata
>> + legal_values { "Generic" "GDB_stubs" }
> Is Generic really an option? Should just be bool surely. Again the ixdp425
> is the same (at least!).
Dunno. This appears in a lot of HALs (ARM/mips/v85x/etc).
>> Index: packages/hal/arm/xscale/prpmc1100/current/include/hal_platform_setup.h
>> ===================================================================
> [snip]
>> + .macro _platform_setup1
>> +
>> +#ifdef CYGINT_HAL_ARM_BIGENDIAN
>> + // set big-endian
>> + mrc p15, 0, r0, c1, c0, 0
>> + orr r0, r0, #0x80
>> + mcr p15, 0, r0, c1, c0, 0
>> + CPWAIT r0
>> +#endif
> CYGINT_HAL_ARM_BIGENDIAN, will always be defined even if it is not
> implemented (it has no flavor that sets it to anything that would cause it
> to be anything different). #if might be better, but in fact if this port
> really is big-endian only you probably want to have the HAL require
> CYGHWR_HAL_ARM_BIGENDIAN == 1 in the arch HAL and #ifdef on that.
The current state of the port is BE, but I think the board could also run LE.
>> +
>> + // Enable the Icache
>> + mrc p15, 0, r0, c1, c0, 0
>> + orr r0, r0, #MMU_Control_I
>> + mcr p15, 0, r0, c1, c0, 0
>> + CPWAIT r0
> #ifdef CYGSEM_HAL_ENABLE_ICACHE_ON_STARTUP ?
okay.
>> +#if defined(CYG_HAL_STARTUP_ROMRAM)
>> + ldr r0,=0x00000000
>> + ldr r1,=0x10000000
>> + ldr r2,=__bss_start
>> + 0: ldr r3,[r0],#4
>> + str r3,[r1],#4
>> + cmp r0,r2
>> + bne 0b
>> +#endif
> Given the lack of ROMRAM MLT, I guess this is just play code, but if not,
> it seems odd since 0x00000000 and 0x10000000 are meant to be just aliases
> of SDRAM.
I used ROMRAM until I found the magic needed to run from flash in a documentation update.
This is just a holdover from that and should be removed.
>> + // enable mmu
>> + mrc p15, 0, r0, c1, c0, 0
>> + orr r0, r0, #MMU_Control_M
>> + orr r0, r0, #MMU_Control_R
>> + mcr p15, 0, r0, c1, c0, 0
>> + CPWAIT r0
> Which reminds me that the common HAL also has
> CYGSEM_HAL_INSTALL_MMU_TABLES, and then CYGSEM_HAL_STATIC_MMU_TABLES can
> be used to set whether to copy the MMU tables into RAM.
I don't think we have a choice on boards supporting flash. The tables have
to be in RAM or a table walk could mess up the flash programming procedure.
>> + // enable D cache
>> + mrc p15, 0, r0, c1, c0, 0
>> + orr r0, r0, #MMU_Control_C
>> + mcr p15, 0, r0, c1, c0, 0
>> + CPWAIT r0
> #ifdef CYGSEM_HAL_ENABLE_DCACHE_ON_STARTUP ?
okay.
>> + // Enable branch target buffer
>> + mrc p15, 0, r0, c1, c0, 0
>> + orr r0, r0, #MMU_Control_BTB
>> + mcr p15, 0, r0, c1, c0, 0
>> + CPWAIT r0
> Arguably #ifdef CYGSEM_HAL_ENABLE_ICACHE_ON_STARTUP ?
probably
>> +// NB: Commented out because of errata on reset function of watchdog timer
>> +//
>> +#if 0
>> +#define HAL_PLATFORM_RESET() \
>> + CYG_MACRO_START \
>> + cyg_uint32 __ctrl; \
>> + /* By disabling interupts we will just hang in the loop below */ \
>> + /* if for some reason the software reset fails. */ \
>> + HAL_DISABLE_INTERRUPTS(__ctrl); \
>> + *IXP425_OST_WDOG_KEY = 0x482e; \
>> + *IXP425_OST_WDOG = 10; \
>> + *IXP425_OST_WDOG_ENA = 5; \
>> + for(;;); /* hang here forever if reset fails */ \
>> + CYG_MACRO_END
>> +#else
>> +#define HAL_PLATFORM_RESET() CYG_EMPTY_STATEMENT
>> +#endif
> Hmm... by doing this it makes RedBoot include the reset command even
> though it will do nothing. The ixp425 HAL has HAL_PLATFORM_RESET_ENTRY set
> to 0x0, but this won't be right without a risk of it going haywire surely?
> (e.g. maybe relative branches around the place assuming it's in ROM when
> in fact it's in RAM).
Its probably not 100% foolproof, but jumping to 0x0 does work. The reset
vector is copied to RAM with the rest and the insn at 0x0 loads the pc
with a 32-bit absolute address which sends it to the right place in ROM.
> I'll change hal/common/current/src/hal_if.c simply not to include the
> reset function if it's not defined by the platform rather than being
> forced to define an empty statement for it.
yeah, this makes sense.
--Mark