| Message ID | 1408424013-20390-1-git-send-email-allen.pais@oracle.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Aug 19, 2014 at 10:23:32AM +0530, Allen Pais wrote: > The following patch adds support for correctly > recognising M7 cpu type. > > Signed-off-by: Allen Pais <allen.pais@oracle.com> > --- > arch/sparc/include/asm/spitfire.h | 1 + > arch/sparc/kernel/cpu.c | 6 ++++++ > arch/sparc/kernel/head_64.S | 13 +++++++++++++ > 3 files changed, 20 insertions(+), 0 deletions(-) > > diff --git a/arch/sparc/include/asm/spitfire.h b/arch/sparc/include/asm/spitfire.h > index 3fc5869..9aec17b 100644 > --- a/arch/sparc/include/asm/spitfire.h > +++ b/arch/sparc/include/asm/spitfire.h > @@ -45,6 +45,7 @@ > #define SUN4V_CHIP_NIAGARA3 0x03 > #define SUN4V_CHIP_NIAGARA4 0x04 > #define SUN4V_CHIP_NIAGARA5 0x05 > +#define SUN4V_CHIP_SPARC_M7 0x08 > #define SUN4V_CHIP_SPARC64X 0x8a > #define SUN4V_CHIP_UNKNOWN 0xff > > diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c > index 82a3a71..55dfb62 100644 > --- a/arch/sparc/kernel/cpu.c > +++ b/arch/sparc/kernel/cpu.c > @@ -494,6 +494,12 @@ static void __init sun4v_cpu_probe(void) > sparc_pmu_type = "niagara5"; > break; > > + case SUN4V_CHIP_SPARC_M7: > + sparc_cpu_type = "SPARC-M7"; > + sparc_fpu_type = "SPARC-M7 integrated FPU"; > + sparc_pmu_type = "sparc-m7"; > + break; > + > case SUN4V_CHIP_SPARC64X: > sparc_cpu_type = "SPARC64-X"; > sparc_fpu_type = "SPARC64-X integrated FPU"; > diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S > index 452f04f..508a542 100644 > --- a/arch/sparc/kernel/head_64.S > +++ b/arch/sparc/kernel/head_64.S > @@ -414,6 +414,7 @@ sun4v_chip_type: > cmp %g2, 'T' > be,pt %xcc, 70f > cmp %g2, 'M' > + be,pt %xcc, 71f > bne,pn %xcc, 49f Looks like you are missing a nop in the delay slot? Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S >> index 452f04f..508a542 100644 >> --- a/arch/sparc/kernel/head_64.S >> +++ b/arch/sparc/kernel/head_64.S >> @@ -414,6 +414,7 @@ sun4v_chip_type: >> cmp %g2, 'T' >> be,pt %xcc, 70f >> cmp %g2, 'M' >> + be,pt %xcc, 71f >> bne,pn %xcc, 49f > Looks like you are missing a nop in the delay slot? I don't think so. I have tested the patch on M7 and I have had no issues with it. - Allen -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 21, 2014 at 03:33:27PM +0530, Allen Pais wrote: > > >> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S > >> index 452f04f..508a542 100644 > >> --- a/arch/sparc/kernel/head_64.S > >> +++ b/arch/sparc/kernel/head_64.S > >> @@ -414,6 +414,7 @@ sun4v_chip_type: > >> cmp %g2, 'T' > >> be,pt %xcc, 70f > >> cmp %g2, 'M' > >> + be,pt %xcc, 71f > >> bne,pn %xcc, 49f > > Looks like you are missing a nop in the delay slot? > > I don't think so. I have tested the patch on M7 and I have had no issues with it. Even if it works as expected then it is confusing. I read the code like this: if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49 Maybe the second opcode is ignored because it is a conditional jump or maybe they are always equal. But whatever it is confusing. Adding an extra nop would help the readability. Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/08/2014 13:14, Sam Ravnborg wrote: > On Thu, Aug 21, 2014 at 03:33:27PM +0530, Allen Pais wrote: >> >>>> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S >>>> index 452f04f..508a542 100644 >>>> --- a/arch/sparc/kernel/head_64.S >>>> +++ b/arch/sparc/kernel/head_64.S >>>> @@ -414,6 +414,7 @@ sun4v_chip_type: >>>> cmp %g2, 'T' >>>> be,pt %xcc, 70f >>>> cmp %g2, 'M' >>>> + be,pt %xcc, 71f >>>> bne,pn %xcc, 49f >>> Looks like you are missing a nop in the delay slot? >> >> I don't think so. I have tested the patch on M7 and I have had no issues with it. > Even if it works as expected then it is confusing. > I read the code like this: > > if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49 > > Maybe the second opcode is ignored because it is a conditional jump or maybe > they are always equal. The two branches are the logical opposite of each other so only one is ever taken and that is why the code "works". > But whatever it is confusing. > > Adding an extra nop would help the readability. > Agreed. This isn't performance sensitive code. Regards Richard P.S. Historical note - SPARC V8 didn't define what happens in the above case as noted on page 77 of the SPARC v9 manual) "V8 Compatibility Note: SPARC-V8 left as undefined the result of executing a delayed conditional branch that had a delayed control transfer in its delay slot. For this reason, programmers should avoid such constructs when backwards compatibility is an issue." -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sam, Richard, >>>>> @@ -414,6 +414,7 @@ sun4v_chip_type: >>>>> cmp %g2, 'T' >>>>> be,pt %xcc, 70f >>>>> cmp %g2, 'M' >>>>> + be,pt %xcc, 71f >>>>> bne,pn %xcc, 49f >>>> Looks like you are missing a nop in the delay slot? >>> >>> I don't think so. I have tested the patch on M7 and I have had no issues with it. >> Even if it works as expected then it is confusing. >> I read the code like this: >> >> if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49 >> >> Maybe the second opcode is ignored because it is a conditional jump or maybe >> they are always equal. > The two branches are the logical opposite of each other so only one is ever taken and that is why the code "works". I'll go ahead and add the delay slot for better readability. It'll look something like this, cmp %g2, 'T' be,pt %xcc, 70f cmp %g2, 'M' + be,pt %xcc, 71f + nop bne,pn %xcc, 49f nop - ALlen -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sam Ravnborg <sam@ravnborg.org> Date: Thu, 21 Aug 2014 14:14:50 +0200 > Maybe the second opcode is ignored because it is a conditional jump > or maybe they are always equal. But whatever it is confusing. I totally agree that the patch should use a nop in the delay slot so that it's clear and easier to understand, however I wanted to show how the behavior is well defined and can even be used intentionally for useful purposes. :-) Branches in a delay slot a long time ago were marked as undefined behavior, but for some time now they are expected to behave in a certain way, consider: ba label_a ba label_b this is a neat way to execute one instruction at label_a then go to label_b. More generically: jmpl %reg ba interpreter_continue interpreter_continue: which you could use to build an instruction at a time execution engine, which only must elide emulate instructions which use registers in this special code sequence. Gordan Irlam used such a technique for his sun4d tracer named SKY. Unfortunately the SKY web page no longer exists, but this tracer led to discoveries that brough about things such as the SLAB allocator. Anyways, all delayed control transfer instructions do is set NPC. So: 0x00 ba 0x20 0x04 ba 0x30 ... 0x20 nop ... 0x30 nop ... At the first instruction, PC=0x00 and NPC=0x04 PC will be set to NPC at the end of execution. For non-branches NPC is set to NPC + 0x4 but for this delayed control transfer NPC will be set to 0x20 instead. So PC=0x04 and NPC=0x20 when we get to the second instruction in the first instruction's delay slot. PC will be set to 0x20 and NPC will be set to 0x30 The nop at 0x20 will be executed, PC will be set to 0x30 and NPC will be set to 0x34. Finally we execute the nop at 0x30 and continue to linearly execute. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Richard Mortimer <richm@oldelvet.org.uk> Date: Thu, 21 Aug 2014 13:29:36 +0100 > On 21/08/2014 13:14, Sam Ravnborg wrote: >> Adding an extra nop would help the readability. >> > Agreed. This isn't performance sensitive code. Alan, please resubmit your patch series with the nop added, thanks. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/sparc/include/asm/spitfire.h b/arch/sparc/include/asm/spitfire.h index 3fc5869..9aec17b 100644 --- a/arch/sparc/include/asm/spitfire.h +++ b/arch/sparc/include/asm/spitfire.h @@ -45,6 +45,7 @@ #define SUN4V_CHIP_NIAGARA3 0x03 #define SUN4V_CHIP_NIAGARA4 0x04 #define SUN4V_CHIP_NIAGARA5 0x05 +#define SUN4V_CHIP_SPARC_M7 0x08 #define SUN4V_CHIP_SPARC64X 0x8a #define SUN4V_CHIP_UNKNOWN 0xff diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c index 82a3a71..55dfb62 100644 --- a/arch/sparc/kernel/cpu.c +++ b/arch/sparc/kernel/cpu.c @@ -494,6 +494,12 @@ static void __init sun4v_cpu_probe(void) sparc_pmu_type = "niagara5"; break; + case SUN4V_CHIP_SPARC_M7: + sparc_cpu_type = "SPARC-M7"; + sparc_fpu_type = "SPARC-M7 integrated FPU"; + sparc_pmu_type = "sparc-m7"; + break; + case SUN4V_CHIP_SPARC64X: sparc_cpu_type = "SPARC64-X"; sparc_fpu_type = "SPARC64-X integrated FPU"; diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S index 452f04f..508a542 100644 --- a/arch/sparc/kernel/head_64.S +++ b/arch/sparc/kernel/head_64.S @@ -414,6 +414,7 @@ sun4v_chip_type: cmp %g2, 'T' be,pt %xcc, 70f cmp %g2, 'M' + be,pt %xcc, 71f bne,pn %xcc, 49f nop @@ -430,6 +431,14 @@ sun4v_chip_type: ba,pt %xcc, 49f nop +71: + ldub [%g1 + 7], %g2 + cmp %g2, '7' + be,pt %xcc, 5f + mov SUN4V_CHIP_SPARC_M7, %g4 + ba,pt %xcc, 49f + nop + 91: sethi %hi(prom_cpu_compatible), %g1 or %g1, %lo(prom_cpu_compatible), %g1 ldub [%g1 + 17], %g2 @@ -586,6 +595,10 @@ niagara_tlb_fixup: be,pt %xcc, niagara4_patch nop + cmp %g1, SUN4V_CHIP_SPARC_M7 + be,pt %xcc, niagara4_patch + nop + call generic_patch_copyops nop call generic_patch_bzero
The following patch adds support for correctly recognising M7 cpu type. Signed-off-by: Allen Pais <allen.pais@oracle.com> --- arch/sparc/include/asm/spitfire.h | 1 + arch/sparc/kernel/cpu.c | 6 ++++++ arch/sparc/kernel/head_64.S | 13 +++++++++++++ 3 files changed, 20 insertions(+), 0 deletions(-)