Re: [Xen-devel] [PATCH V3] xen: arm: introduce Cortex-A7 support

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 17:47 +0800 on 09 Jul (1373392034), Bamvor Jian Zhang wrote:
>          PRINT("- Setting up control registers -\r\n")
>  
> -        /* Read CPU ID */
> -        mrc   CP32(r0, MIDR)
> -        ldr   r1, =(MIDR_MASK)
> -        and   r0, r0, r1
> -        /* Is this a Cortex A15? */
> -        ldr   r1, =(CORTEX_A15_ID)
> -        teq   r0, r1
> -        bleq  cortex_a15_init
> +        /* Get processor specific proc info into r1 */
> +        mrc   CP32(r0, MIDR)                /* r0 := our cpu id */
> +        ldr   r1, = __proc_info_start
> +        add   r1, r1, r10                   /* r1 := paddr of table (start) 
> */
> +        ldr   r2, = __proc_info_end
> +        add   r2, r2, r10                   /* r2 := paddr of table (end) */
> +1:      ldr   r3, [r1, #PROCINFO_cpu_mask]
> +        and   r0, r0, r3

This mask clobbers the only copy of the CPU ID, so the second and
subsequent checks will be wrong.  You need to keep a copy of the
original around or read MIDR inside the loop.

> +        ldr   r3, [r1, #PROCINFO_cpu_val]   /* r3 := cpu val in current proc 
> info */
> +        teq   r0, r3
> +        beq   2f                            /* Match => exit, or try next 
> proc info */
> +        add   r1, r1, #PROCINFO_sizeof
> +        cmp   r1, r2
> +        blo   1b
> +        mov   r4, r0
> +        PRINT("- Missing processor info: ")
> +        mov   r0, r4

It would be good to print the un-masked ID here too. 

> +        bl    putn
> +        PRINT(" -\r\n")
> +        b     fail
> +
> +2:
> +        /* Jump to cpu_init */
> +        ldr   r1, [r1, #PROCINFO_cpu_init]  /* r1 := vaddr(init func) */
> +        adr   lr, cpu_init_done             /* Save return address */
> +        add   pc, r1, r10                   /* Call paddr(init func) */
>  
> +cpu_init_done:
[...]
> +.globl v7_init
> +v7_init:
> +        /* Set up the SMP bit in ACTLR */
> +        mrc   CP32(r0, ACTLR)
> +        orr   r0, r0, #(ACTLR_V7_SMP) /* enable SMP bit */
> +        mcr   CP32(r0, ACTLR)
> +        mov   pc, lr
> +
> +        .section ".init.proc.info", #alloc, #execinstr
> +        .type __v7_ca15mp_proc_info, #object
> +__v7_ca15mp_proc_info:
> +        .long 0x410FC0F0             /* Cortex-A15 */
> +        .long 0xFF0FFFF0             /* Mask */
> +        .long v7_init
> +        .size __v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
> +
> +        .section ".init.proc.info", #alloc, #execinstr
> +        .type __v7_ca7mp_proc_info, #object
> +__v7_ca7mp_proc_info:
> +        .long 0x410FC070             /* Cortex-A7 */
> +        .long 0xFF0FFFF0             /* Mask */
> +        .long v7_init
> +        .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info
> +

I'd asked for these to be defined in C -- that way all the OFFSET_
macros in head.S are guaranteed to be right.  But OTOH it's nice to have
them here in the same file as the code they reference, so I guess it's
OK this way too.

The rest of this looks good to me.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel