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

["Bamvor Jian Zhang" <bjzhang@xxxxxxxx>]

Hi, Ian

 >>>Wrote "Ian Campbell <Ian.Campbell@xxxxxxxxxx>"> On Tue,
 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?
yes
> Can the initial comment say "Get processor specific proc info into rX" please.
sure
> 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.
ok. i will
> 
> >  
> > +        /* 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).
ok
> Since we already have physoffset in r10 would it be clearer to make use
> of that?
sorry if i am wrong. do u mean change "adr lr, 1f" to something like 
"add lr, pc, r10"?
> 
> > +        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.
Ok. 
So, i will remove 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.
ok.
> 
> [...]
> > +#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.
yes, i need to do this for Aarch64 extesion in future.
> 
> > +   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.
i was thinking about it. if i define the following
#define ACTLR_CA15_SMP                ACTLR_V7_SMP
i need include asm/arm/processor.h which including CP micros.

Bamvor
> 
> >  #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