Re: [Xen-devel] [PATCH V2 1/2] xen: arm: introduce Cortex-A7 support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-07-02 at 21:11 +0800, Bamvor Jian Zhang wrote:
> Introduce Cortex-A7 with a scalable proc_info_list which including cpu id
> and cpu initialize function.
> In head.S, search cpu specific MIDR in procinfo and call such initialize
> function. Currently, support Cortex-A7 and Cortex-A15.

I like the general shape of it, thanks!

A few comments below.

> Signed-off-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>

> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 0588d54..d62401d 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -20,6 +20,7 @@
>  #include <asm/config.h>
>  #include <asm/page.h>
>  #include <asm/processor-ca15.h>
> +#include <asm/processor-ca7.h>
>  #include <asm/asm_defns.h>
>  
>  #define ZIMAGE_MAGIC_NUMBER 0x016f2818
> @@ -188,15 +189,33 @@ skip_bss:
>  
>          PRINT("- Setting up control registers -\r\n")
>  
> -        /* Read CPU ID */
> +        /* Get processor specific proc info */
>          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
> +        ldr   r1, = __proc_info_start
> +        add   r1, r1, r10
> +        ldr   r2, = __proc_info_end
> +        add   r2, r2, r10
> +1:      ldr   r3, [r1]
> +        teq   r0, r3
> +        beq   2f
> +        add   r1, r1, #PROCINFO_sizeof
> +        cmp   r1, r2
> +        blo   1b
> +        mov   r4, r0
> +        PRINT("- Missing processor info: ")
> +        mov   r0, r4
> +        bl    putn
> +        PRINT(" -\r\n")
> +        b     fail
> +2:

At the end of all this r1 == physical address of the platform
information? Can the initial comment say "Get processor specific proc
info into rX" please.

If you could also follow the lead of other code in this file and
obsessively comment what it is doing that would be great. e.g. comments
like "r1 := physical address of start of table", "r2 := physical address
of end of table" etc.

>  
> +        /* Set return address(PIC) */
> +        /* XXX: it only work while thumb2 is not enable in xen */

That's true of lots/all of our asm. Lets ignore that for now (no need to
comment).

Since we already have physoffset in r10 would it be clearer to make use
of that?

> +        adr   lr, 1f
> +        add   pc, r1, #PROCINFO_cpu_init 
> +1:
>          /* Set up memory attribute type tables */
>          ldr   r0, =MAIR0VAL
>          ldr   r1, =MAIR1VAL

> diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S
> new file mode 100644
> index 0000000..2d46dca
> --- /dev/null
> +++ b/xen/arch/arm/arm32/proc-v7.S
> @@ -0,0 +1,36 @@
[...]
> +#include <asm/asm_defns.h>
> +#include <asm/arm32/processor.h>
> +
> +.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

Lets put the __v7_ca7mp_proc_info and __v7_ca15mp_proc_info here,
calling v7_init directly and do away with proc-ca15.S and proc-ca7.S.

> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */

> diff --git a/xen/include/asm-arm/arm32/procinfo.h 
> b/xen/include/asm-arm/arm32/procinfo.h
> new file mode 100644
> index 0000000..7e7a775
> --- /dev/null
> +++ b/xen/include/asm-arm/arm32/procinfo.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/asm-arm/arm32/procinfo.h

I suggest to put this in just include/asm-arm/procinfo.h. The same
struct should eventually apply to 64-bit too.

[...]
> +#ifndef __ASM_ARM_ARM32_PROCINFO_H
> +#define __ASM_ARM_ARM32_PROCINFO_H
> +
> +struct proc_info_list {
> +     unsigned int            cpu_val;

I think we should include "unsigned int cpu_mask" here too and remove
MIDR_MASK. Will need equivalent changes in head.S as well.

> +     unsigned long           cpu_init;        /* used by head.S */
> +};
> +
> +#endif
> diff --git a/xen/include/asm-arm/processor-ca15.h 
> b/xen/include/asm-arm/processor-ca15.h
> index 06cdbdd..96438f0 100644
> --- a/xen/include/asm-arm/processor-ca15.h
> +++ b/xen/include/asm-arm/processor-ca15.h
> @@ -1,9 +1,6 @@
>  #ifndef __ASM_ARM_PROCESSOR_CA15_H
>  #define __ASM_ARM_PROCESSOR_CA15_H
>  
> -
> -#define CORTEX_A15_ID     (0x410FC0F0)
> -
>  /* ACTLR Auxiliary Control Register, Cortex A15 */
>  #define ACTLR_CA15_SNOOP_DELAYED      (1<<31)
>  #define ACTLR_CA15_MAIN_CLOCK         (1<<30)
> @@ -26,7 +23,6 @@
>  #define ACTLR_CA15_OPT                (1<<9)
>  #define ACTLR_CA15_WFI                (1<<8)
>  #define ACTLR_CA15_WFE                (1<<7)
> -#define ACTLR_CA15_SMP                (1<<6)

Lets leave this for completeness even if we aren't using it.

>  #define ACTLR_CA15_PLD                (1<<5)
>  #define ACTLR_CA15_IP                 (1<<4)
>  #define ACTLR_CA15_MICRO_BTB          (1<<3)



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