Re: PrPMC1100 port

[Jonathan Larmour <jifl at eCosCentric dot com>]

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.

> +    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!).

> 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.

> +
> +	// 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 ?

> +#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.

> +        // 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.

> +        // 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 ?


> +        // 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 ?

> +// 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).

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.

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine