Xen devel - Oct 2013 - [PATCH] xen/arm: Add CPU ID for Broadcom Brahma-B15

From: Marc Carino <marc.ceeeee@gmail.com>

Let Xen recognize the Broadcom Brahma-B15 CPU by adding the appropriate
MIDR mask to the initialization phase. Further, ensure that the console
output properly reports the CPU manufacturer as "Broadcom
Corporation".

Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
---
 xen/arch/arm/arm32/proc-v7.S |    8 ++++++++
 xen/arch/arm/setup.c         |    1 +
 2 files changed, 9 insertions(+)

diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S
index 6577a89..2c8cb9c 100644
--- a/xen/arch/arm/arm32/proc-v7.S
+++ b/xen/arch/arm/arm32/proc-v7.S
@@ -43,6 +43,14 @@ __v7_ca7mp_proc_info:
         .long v7_init
         .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info
 
+        .section ".init.proc.info", #alloc, #execinstr
+        .type __v7_brahma15mp_proc_info, #object
+__v7_brahma15mp_proc_info:
+        .long 0x420F00F2             /* Broadcom Brahma-B15 */
+        .long 0xFF0FFFFF             /* Mask */
+        .long v7_init
+        .size __v7_brahma15mp_proc_info, . - __v7_brahma15mp_proc_info
+
 /*
  * Local variables:
  * mode: ASM
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 49f344c..7d6e596 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -61,6 +61,7 @@ static void __init init_idle_domain(void)
 
 static const char * __initdata processor_implementers[] = {
     [''A''] = "ARM Limited",
+    [''B''] = "Broadcom Corporation",
     [''D''] = "Digital Equipment Corp",
     [''M''] = "Motorola, Freescale Semiconductor
Inc.",
     [''Q''] = "Qualcomm Inc.",
-- 
1.7.9.5

On Wed, 2013-10-16 at 15:57 -0700, marc.ceeeee@gmail.com
wrote:
> From: Marc Carino <marc.ceeeee@gmail.com>
> 
> Let Xen recognize the Broadcom Brahma-B15 CPU by adding the appropriate
> MIDR mask to the initialization phase. Further, ensure that the console
> output properly reports the CPU manufacturer as "Broadcom
Corporation".
Thanks for resending, I saw the original but somehow failed to add it to
my queue of things to look at, sorry!
> 
> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
> ---
>  xen/arch/arm/arm32/proc-v7.S |    8 ++++++++
>  xen/arch/arm/setup.c         |    1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S
> index 6577a89..2c8cb9c 100644
> --- a/xen/arch/arm/arm32/proc-v7.S
> +++ b/xen/arch/arm/arm32/proc-v7.S
> @@ -43,6 +43,14 @@ __v7_ca7mp_proc_info:
>          .long v7_init
>          .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info
>  
> +        .section ".init.proc.info", #alloc, #execinstr
> +        .type __v7_brahma15mp_proc_info, #object
> +__v7_brahma15mp_proc_info:
> +        .long 0x420F00F2             /* Broadcom Brahma-B15 */
> +        .long 0xFF0FFFFF             /* Mask */
This mask includes the revision field (last nibble). Is it intentional
too only support a single revision of this processor?
> +        .long v7_init
v7_init fiddles with the implementation defined ACTLR register in a
Cortex-A15/A7 specific manner. Is the B15 compatible with ARM''s Cortex
A15 in this regard?

If not then I think this should be changed to:

ca7mp_init:
ca15mp_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)
v7_init: /* Generic v7 init */
        mov   pc, lr

and the A15/A7 entries updated to point to the new names.

Perhaps there isn''t much point in listing a processor here which
doesn''t
need any init, but doing so will mean it''ll get picked up if we ever
add
some init for generic v7, or if we add new hooks etc.

Ian.
> +        .size __v7_brahma15mp_proc_info, . - __v7_brahma15mp_proc_info
> +
>  /*
>   * Local variables:
>   * mode: ASM
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 49f344c..7d6e596 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -61,6 +61,7 @@ static void __init init_idle_domain(void)
>  
>  static const char * __initdata processor_implementers[] = {
>      [''A''] = "ARM Limited",
> +    [''B''] = "Broadcom Corporation",
>      [''D''] = "Digital Equipment Corp",
>      [''M''] = "Motorola, Freescale Semiconductor
Inc.",
>      [''Q''] = "Qualcomm Inc.",

Hello Ian,
> Thanks for resending, I saw the original but somehow failed to add it to
> my queue of things to look at, sorry!
No problem. Thanks for taking the time to look at the patch!
>> +        .long 0x420F00F2             /* Broadcom Brahma-B15 */
>> +        .long 0xFF0FFFFF             /* Mask */
>
> This mask includes the revision field (last nibble). Is it intentional
> too only support a single revision of this processor?
Yes. At this time, supporting only this specific version of this CPU
is required.
>> +        .long v7_init
>
> v7_init fiddles with the implementation defined ACTLR register in a
> Cortex-A15/A7 specific manner. Is the B15 compatible with ARM''s
Cortex
> A15 in this regard?
Correct. The B15 will also require the SMP bit to be OR''ed in ACTLR,
so they can be considered compatible in this regard.

I could go ahead with refactoring the code in proc-v7.S, especially if
you expect there to be any future ARMv7 chips with unique
initialization requirements. I think going with your suggestion would
align the code with the Linux kernel''s arch/arm/mm/proc-v7.S, which I
assume is a good thing!

Thanks,
Marc

On Thu, Oct 17, 2013 at 2:47 AM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Wed, 2013-10-16 at 15:57 -0700, marc.ceeeee@gmail.com wrote:
>> From: Marc Carino <marc.ceeeee@gmail.com>
>>
>> Let Xen recognize the Broadcom Brahma-B15 CPU by adding the appropriate
>> MIDR mask to the initialization phase. Further, ensure that the console
>> output properly reports the CPU manufacturer as "Broadcom
Corporation".
>
> Thanks for resending, I saw the original but somehow failed to add it to
> my queue of things to look at, sorry!
>
>>
>> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
>> ---
>>  xen/arch/arm/arm32/proc-v7.S |    8 ++++++++
>>  xen/arch/arm/setup.c         |    1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/proc-v7.S
b/xen/arch/arm/arm32/proc-v7.S
>> index 6577a89..2c8cb9c 100644
>> --- a/xen/arch/arm/arm32/proc-v7.S
>> +++ b/xen/arch/arm/arm32/proc-v7.S
>> @@ -43,6 +43,14 @@ __v7_ca7mp_proc_info:
>>          .long v7_init
>>          .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info
>>
>> +        .section ".init.proc.info", #alloc, #execinstr
>> +        .type __v7_brahma15mp_proc_info, #object
>> +__v7_brahma15mp_proc_info:
>> +        .long 0x420F00F2             /* Broadcom Brahma-B15 */
>> +        .long 0xFF0FFFFF             /* Mask */
>
> This mask includes the revision field (last nibble). Is it intentional
> too only support a single revision of this processor?
>
>> +        .long v7_init
>
> v7_init fiddles with the implementation defined ACTLR register in a
> Cortex-A15/A7 specific manner. Is the B15 compatible with ARM''s
Cortex
> A15 in this regard?
>
> If not then I think this should be changed to:
>
> ca7mp_init:
> ca15mp_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)
> v7_init: /* Generic v7 init */
>         mov   pc, lr
>
> and the A15/A7 entries updated to point to the new names.
>
> Perhaps there isn''t much point in listing a processor here which
doesn''t
> need any init, but doing so will mean it''ll get picked up if we
ever add
> some init for generic v7, or if we add new hooks etc.
>
> Ian.
>
>> +        .size __v7_brahma15mp_proc_info, . - __v7_brahma15mp_proc_info
>> +
>>  /*
>>   * Local variables:
>>   * mode: ASM
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 49f344c..7d6e596 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -61,6 +61,7 @@ static void __init init_idle_domain(void)
>>
>>  static const char * __initdata processor_implementers[] = {
>>      [''A''] = "ARM Limited",
>> +    [''B''] = "Broadcom Corporation",
>>      [''D''] = "Digital Equipment Corp",
>>      [''M''] = "Motorola, Freescale Semiconductor
Inc.",
>>      [''Q''] = "Qualcomm Inc.",
>
>

On Thu, 2013-10-17 at 16:13 -0700, Marc C wrote:
> Hello Ian,
> 
> > Thanks for resending, I saw the original but somehow failed to add it
to
> > my queue of things to look at, sorry!
> 
> No problem. Thanks for taking the time to look at the patch!
> 
> >> +        .long 0x420F00F2             /* Broadcom Brahma-B15 */
> >> +        .long 0xFF0FFFFF             /* Mask */
> >
> > This mask includes the revision field (last nibble). Is it intentional
> > too only support a single revision of this processor?
> 
> Yes. At this time, supporting only this specific version of this CPU
> is required.
Is there any harm in allowing all of them though?
> >> +        .long v7_init
> >
> > v7_init fiddles with the implementation defined ACTLR register in a
> > Cortex-A15/A7 specific manner. Is the B15 compatible with
ARM''s Cortex
> > A15 in this regard?
> 
> Correct. The B15 will also require the SMP bit to be OR''ed in
ACTLR,
> so they can be considered compatible in this regard.
> 
> I could go ahead with refactoring the code in proc-v7.S,
Given the above it''s not a requirement so I''ll apply this
patch as is,
but if you feel inclined to follow up with the refactoring that would be
wonderful.

Thanks,
Ian.