[PATCH] ARM: proc: Add Krait proc info

[PATCH] ARM: proc: Add Krait proc info

[Stephen Boyd]

From: Stepan Moskovchenko <stepanm@codeaurora.org>

Add processor info for the Qualcomm, Inc. Krait family of
processors, to use the generic ARMv7 initialisation
procedure but explicitly enable the IDIV hardware
capability flag.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/mm/proc-v7.S |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 3a3c015..81adeb3 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -434,6 +434,16 @@ __v7_ca15mp_proc_info:
 	.size	__v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
 
 	/*
+	 * Qualcomm Inc. Krait processors.
+	 */
+	.type	__krait_proc_info, #object
+__krait_proc_info:
+	.long	0x510f0400		@ Required ID value
+	.long	0xff0ffc00		@ Mask for ID
+	__v7_proc __v7_setup, hwcaps = HWCAP_IDIV
+	.size	__krait_proc_info, . - __krait_proc_info
+
+	/*
 	 * Match any ARMv7 processor core.
 	 */
 	.type	__v7_proc_info, #object
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] ARM: proc: Add Krait proc info

[Will Deacon]

Hi Stephen, Stepan,

On Mon, Mar 04, 2013 at 11:21:39PM +0000, Stephen Boyd wrote:
> From: Stepan Moskovchenko <stepanm@codeaurora.org>
> 
> Add processor info for the Qualcomm, Inc. Krait family of
> processors, to use the generic ARMv7 initialisation
> procedure but explicitly enable the IDIV hardware
> capability flag.
> 
> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  arch/arm/mm/proc-v7.S |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 3a3c015..81adeb3 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -434,6 +434,16 @@ __v7_ca15mp_proc_info:
>  	.size	__v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
>  
>  	/*
> +	 * Qualcomm Inc. Krait processors.
> +	 */
> +	.type	__krait_proc_info, #object
> +__krait_proc_info:
> +	.long	0x510f0400		@ Required ID value
> +	.long	0xff0ffc00		@ Mask for ID
> +	__v7_proc __v7_setup, hwcaps = HWCAP_IDIV

I was looking at this the other day and wondered whether we could set
HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
think why that would be difficult, but similarly there may well be a reason
why we assign it like this.

Fancy taking a look?

Will

Re: [PATCH] ARM: proc: Add Krait proc info

[Stephen Boyd]

On 03/05/13 00:34, Will Deacon wrote:
> Hi Stephen, Stepan,
>
> On Mon, Mar 04, 2013 at 11:21:39PM +0000, Stephen Boyd wrote:
>> From: Stepan Moskovchenko <stepanm@codeaurora.org>
>>
>> Add processor info for the Qualcomm, Inc. Krait family of
>> processors, to use the generic ARMv7 initialisation
>> procedure but explicitly enable the IDIV hardware
>> capability flag.
>>
>> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>  arch/arm/mm/proc-v7.S |   10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
>> index 3a3c015..81adeb3 100644
>> --- a/arch/arm/mm/proc-v7.S
>> +++ b/arch/arm/mm/proc-v7.S
>> @@ -434,6 +434,16 @@ __v7_ca15mp_proc_info:
>>  	.size	__v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
>>  
>>  	/*
>> +	 * Qualcomm Inc. Krait processors.
>> +	 */
>> +	.type	__krait_proc_info, #object
>> +__krait_proc_info:
>> +	.long	0x510f0400		@ Required ID value
>> +	.long	0xff0ffc00		@ Mask for ID
>> +	__v7_proc __v7_setup, hwcaps = HWCAP_IDIV
> I was looking at this the other day and wondered whether we could set
> HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
> think why that would be difficult, but similarly there may well be a reason
> why we assign it like this.
>
> Fancy taking a look?

Ok I'll take a look. Should we be masking out HWCAP_IDIVT on
!CONFIG_ARM_THUMB configurations?

----8<-----

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 3f6cbb2..e2c8bbf 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -484,7 +484,7 @@ static void __init setup_processor(void)
                 list->elf_name, ENDIANNESS);
        elf_hwcap = list->elf_hwcap;
 #ifndef CONFIG_ARM_THUMB
-       elf_hwcap &= ~HWCAP_THUMB;
+       elf_hwcap &= ~(HWCAP_THUMB | HWCAP_IDIVT);
 #endif
 
        feat_v6_fixup();

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] ARM: proc: Add Krait proc info

[Will Deacon]

On Tue, Mar 05, 2013 at 10:03:10PM +0000, Stephen Boyd wrote:
> On 03/05/13 00:34, Will Deacon wrote:
> > I was looking at this the other day and wondered whether we could set
> > HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
> > think why that would be difficult, but similarly there may well be a reason
> > why we assign it like this.
> >
> > Fancy taking a look?
> 
> Ok I'll take a look. Should we be masking out HWCAP_IDIVT on
> !CONFIG_ARM_THUMB configurations?

Yes, that's not a bad idea. It makes the IDIV* caps follow what we do for
the others (vfp etc) by only advertising support if the relevant CONFIG
options are selected.

Will

Re: [PATCH] ARM: proc: Add Krait proc info

[Stephen Boyd]

On 03/05/13 14:03, Stephen Boyd wrote:
> On 03/05/13 00:34, Will Deacon wrote:
>> Hi Stephen, Stepan,
>>
>> On Mon, Mar 04, 2013 at 11:21:39PM +0000, Stephen Boyd wrote:
>>> From: Stepan Moskovchenko <stepanm@codeaurora.org>
>>>
>>> Add processor info for the Qualcomm, Inc. Krait family of
>>> processors, to use the generic ARMv7 initialisation
>>> procedure but explicitly enable the IDIV hardware
>>> capability flag.
>>>
>>> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>> ---
>>>  arch/arm/mm/proc-v7.S |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
>>> index 3a3c015..81adeb3 100644
>>> --- a/arch/arm/mm/proc-v7.S
>>> +++ b/arch/arm/mm/proc-v7.S
>>> @@ -434,6 +434,16 @@ __v7_ca15mp_proc_info:
>>>  	.size	__v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
>>>  
>>>  	/*
>>> +	 * Qualcomm Inc. Krait processors.
>>> +	 */
>>> +	.type	__krait_proc_info, #object
>>> +__krait_proc_info:
>>> +	.long	0x510f0400		@ Required ID value
>>> +	.long	0xff0ffc00		@ Mask for ID
>>> +	__v7_proc __v7_setup, hwcaps = HWCAP_IDIV
>> I was looking at this the other day and wondered whether we could set
>> HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
>> think why that would be difficult, but similarly there may well be a reason
>> why we assign it like this.
>>
>> Fancy taking a look?
> Ok I'll take a look. 

Hmm. I wonder if we did it this way because between version B and C of
DDI0406 the definition of those bits changed.

In DDI0406B we have

    0 - no support
    1 - support

and in DDI0406C we have

    0 - no support
    1 - support in Thumb
    2 - support in Thumb and ARM

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] ARM: proc: Add Krait proc info

[Will Deacon]

On Wed, Mar 06, 2013 at 05:20:32AM +0000, Stephen Boyd wrote:
> On 03/05/13 14:03, Stephen Boyd wrote:
> > On 03/05/13 00:34, Will Deacon wrote:
> >> I was looking at this the other day and wondered whether we could set
> >> HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
> >> think why that would be difficult, but similarly there may well be a reason
> >> why we assign it like this.
> >>
> >> Fancy taking a look?
> > Ok I'll take a look. 
> 
> Hmm. I wonder if we did it this way because between version B and C of
> DDI0406 the definition of those bits changed.
> 
> In DDI0406B we have
> 
>     0 - no support
>     1 - support
> 
> and in DDI0406C we have
> 
>     0 - no support
>     1 - support in Thumb
>     2 - support in Thumb and ARM

Well spotted, although I think this a documentation error. I've checked both
A7 and A15 and they both advertise '2' (although r0p0 TRM for A7 also gets
this wrong, the CPU does the right thing).

What about the Qualcomm CPUs?

Will

Re: [PATCH] ARM: proc: Add Krait proc info

[Stephen Boyd]

On 03/07/13 22:41, Will Deacon wrote:
> On Wed, Mar 06, 2013 at 05:20:32AM +0000, Stephen Boyd wrote:
>> On 03/05/13 14:03, Stephen Boyd wrote:
>>> On 03/05/13 00:34, Will Deacon wrote:
>>>> I was looking at this the other day and wondered whether we could set
>>>> HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
>>>> think why that would be difficult, but similarly there may well be a reason
>>>> why we assign it like this.
>>>>
>>>> Fancy taking a look?
>>> Ok I'll take a look. 
>> Hmm. I wonder if we did it this way because between version B and C of
>> DDI0406 the definition of those bits changed.
>>
>> In DDI0406B we have
>>
>>     0 - no support
>>     1 - support
>>
>> and in DDI0406C we have
>>
>>     0 - no support
>>     1 - support in Thumb
>>     2 - support in Thumb and ARM
> Well spotted, although I think this a documentation error. I've checked both
> A7 and A15 and they both advertise '2' (although r0p0 TRM for A7 also gets
> this wrong, the CPU does the right thing).

What about the Cortex-R7? When I google "ARM ISAR0" the first hit is

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0363e/Bgbfafej.html

and in there it just says 1.

> What about the Qualcomm CPUs?
>

It's set to 1 in some of our earlier Krait CPU designs, but we corrected
it later on to say 2 because the hardware actually supports the
instructions in both ARM and Thumb mode. Unfortunately, we mass produced
some of these chips that say 1 so we still need to do the same kind of
MIDR check. I'm going to send the patchset right now with all this in it.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] ARM: proc: Add Krait proc info

[Jonathan Austin]

On 13/03/13 01:31, Stephen Boyd wrote:
> On 03/07/13 22:41, Will Deacon wrote:
>> On Wed, Mar 06, 2013 at 05:20:32AM +0000, Stephen Boyd wrote:
>>> On 03/05/13 14:03, Stephen Boyd wrote:
>>>> On 03/05/13 00:34, Will Deacon wrote:
>>>>> I was looking at this the other day and wondered whether we could set
>>>>> HWCAP_IDIV in __v7_setup, depending on ID_ISAR0[27:24]. I can't immediately
>>>>> think why that would be difficult, but similarly there may well be a reason
>>>>> why we assign it like this.
>>>>>
>>>>> Fancy taking a look?
>>>> Ok I'll take a look.
>>> Hmm. I wonder if we did it this way because between version B and C of
>>> DDI0406 the definition of those bits changed.
>>>
>>> In DDI0406B we have
>>>
>>>      0 - no support
>>>      1 - support
>>>
>>> and in DDI0406C we have
>>>
>>>      0 - no support
>>>      1 - support in Thumb
>>>      2 - support in Thumb and ARM
>> Well spotted, although I think this a documentation error. I've checked both
>> A7 and A15 and they both advertise '2' (although r0p0 TRM for A7 also gets
>> this wrong, the CPU does the right thing).
>
> What about the Cortex-R7? When I google "ARM ISAR0" the first hit is
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0363e/Bgbfafej.html
>

That appears to be an R4 TRM, but I've checked the details for R4, 5 and 7.

In R4 there is no ARM hardware divide, so the document that you linked 
to is correct in specifying only 0b1

In R5, ARM hardware divide was added in r0p1, and the value of 
ID_ISAR0[27:24] is updated to 0b10 accordingly

In R7, the TRM just refers you to the ARMARM, but the hardware guys 
assure me that ID_ISAR0[27:24] will be accurately 1, or 2 depending on 
what the processor actually supports.

Hope that helps,
Jonny