This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]