| Message ID | 1467004063-22417-1-git-send-email-oohall@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
* Oliver O'Halloran <oohall@gmail.com> [2016-06-27 15:07:43]: > ISAv3 adds a mode to increase the size of the decrementer from 32 bits. > The enlarged decrementer can be between 32 and 64 bits wide with the > exact value being implementation dependent. This patch adds support for > detecting the size of the large decrementer and populating each CPU node > with the "ibm,dec-bits" property. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> > --- > core/cpu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/processor.h | 1 + > 2 files changed, 49 insertions(+) > > diff --git a/core/cpu.c b/core/cpu.c > index ffc264f90907..32b70e0c3713 100644 > --- a/core/cpu.c > +++ b/core/cpu.c > @@ -524,10 +524,55 @@ void init_boot_cpu(void) > list_head_init(&global_job_queue); > } > > +static void enable_ld(bool on) > +{ > + u64 lpcr = mfspr(SPR_LPCR); > + > + if (on) > + lpcr |= SPR_LPCR_P9_LD; > + else > + lpcr &= ~SPR_LPCR_P9_LD; > + > + mtspr(SPR_LPCR, lpcr); > +} How about expanding 'ld' in the function name and also maybe have enable/disable for better readability instead of taking a parameter. static inline void enable_long_dec() static inline void disable_long_dec() > + > +#define HIGH_BIT (1ull << 63) > + > +static int find_dec_bits(void) > +{ > + int bits = 65; /* we always decrement once */ > + u64 mask = ~0ull; > + > + if (proc_gen < proc_gen_p9) > + return 32; > + > + /* The ISA doesn't specify the width of the decrementer register so we > + * need to discover it. When in large mode (LPCR.LD = 1) reads from the > + * DEC SPR are sign extended to 64 bits and writes are truncated to the > + * physical register width. We can use this behaviour to detect the > + * width by starting from an all 1s value and left shifting until we > + * read a value from the DEC with it's high bit cleared. > + */ > + > + enable_ld(true); > + > + do { > + bits--; > + mask = mask >> 1; > + mtspr(SPR_DEC, mask); > + } while (mfspr(SPR_DEC) & HIGH_BIT); > + > + enable_ld(false); > + > + prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); > + return bits; Number of bits supported by the decrementer is one more than the number of bits set in the mask at the time when sign extension stops and HIGH_BIT is not set. Will this be true always? Also the decrementer would count down from the mask value before we compare, but that will not toggle the higher order bits and the loop will work correctly :) > +} > + > void init_all_cpus(void) > { > struct dt_node *cpus, *cpu; > unsigned int thread, new_max_pir = 0; > + int dec_bits = find_dec_bits(); You already test function of large decrementer and only then export ibm,dec-bits > 32. You leave LD disabled in LPCR and let the Linux kernel check and enable. Why do we have to test the number of bits again in kernel? Do you have a reason to believe that test/check in skiboot is not sufficient? You have check_large_decrementer() in kernel that will again validate the number of bits that you have just discovered. > cpus = dt_find_by_path(dt_root, "/cpus"); > assert(cpus); > @@ -582,6 +627,9 @@ void init_all_cpus(void) > /* Add associativity properties */ > add_core_associativity(t); > > + /* Add the decrementer width property */ > + dt_add_property_cells(cpu, "ibm,dec-bits", dec_bits); > + > /* Adjust max PIR */ > if (new_max_pir < (pir + cpu_thread_count - 1)) > new_max_pir = pir + cpu_thread_count - 1; > diff --git a/include/processor.h b/include/processor.h > index 1236c779aac8..48bbf903df58 100644 > --- a/include/processor.h > +++ b/include/processor.h > @@ -96,6 +96,7 @@ > #define SPR_LPCR_P8_PECE2 PPC_BIT(49) /* Wake on external interrupts */ > #define SPR_LPCR_P8_PECE3 PPC_BIT(50) /* Wake on decrementer */ > #define SPR_LPCR_P8_PECE4 PPC_BIT(51) /* Wake on MCs, HMIs, etc... */ > +#define SPR_LPCR_P9_LD PPC_BIT(46) /* Large decrementer mode bit */ > > > /* Bits in TFMR - control bits */ > -- > 2.5.5 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Tue, Jun 28, 2016 at 1:31 AM, Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> wrote: > * Oliver O'Halloran <oohall@gmail.com> [2016-06-27 15:07:43]: > >> ISAv3 adds a mode to increase the size of the decrementer from 32 bits. >> The enlarged decrementer can be between 32 and 64 bits wide with the >> exact value being implementation dependent. This patch adds support for >> detecting the size of the large decrementer and populating each CPU node >> with the "ibm,dec-bits" property. >> >> Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > > Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> > >> --- >> core/cpu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> include/processor.h | 1 + >> 2 files changed, 49 insertions(+) >> >> diff --git a/core/cpu.c b/core/cpu.c >> index ffc264f90907..32b70e0c3713 100644 >> --- a/core/cpu.c >> +++ b/core/cpu.c >> @@ -524,10 +524,55 @@ void init_boot_cpu(void) >> list_head_init(&global_job_queue); >> } >> >> +static void enable_ld(bool on) >> +{ >> + u64 lpcr = mfspr(SPR_LPCR); >> + >> + if (on) >> + lpcr |= SPR_LPCR_P9_LD; >> + else >> + lpcr &= ~SPR_LPCR_P9_LD; >> + >> + mtspr(SPR_LPCR, lpcr); >> +} > > How about expanding 'ld' in the function name and also maybe have > enable/disable for better readability instead of taking a parameter. > > static inline void enable_long_dec() > static inline void disable_long_dec() I have no real preference. I thought having seperate functions just resulted in two (ugly) sets of mfspr/bit twiddle/mtspr sequences rather than this, which I thought was pretty clean. I suppose it could have a better name... > >> + >> +#define HIGH_BIT (1ull << 63) >> + >> +static int find_dec_bits(void) >> +{ >> + int bits = 65; /* we always decrement once */ >> + u64 mask = ~0ull; >> + >> + if (proc_gen < proc_gen_p9) >> + return 32; >> + >> + /* The ISA doesn't specify the width of the decrementer register so we >> + * need to discover it. When in large mode (LPCR.LD = 1) reads from the >> + * DEC SPR are sign extended to 64 bits and writes are truncated to the >> + * physical register width. We can use this behaviour to detect the >> + * width by starting from an all 1s value and left shifting until we >> + * read a value from the DEC with it's high bit cleared. >> + */ >> + >> + enable_ld(true); >> + >> + do { >> + bits--; >> + mask = mask >> 1; >> + mtspr(SPR_DEC, mask); >> + } while (mfspr(SPR_DEC) & HIGH_BIT); >> + >> + enable_ld(false); >> + >> + prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); >> + return bits; > > Number of bits supported by the decrementer is one more than the > number of bits set in the mask at the time when sign extension stops > and HIGH_BIT is not set. Will this be true always? The ISA says "When the Decrementer is in Large Decrementer mode, it behaves as a d-bit decrementing counter which is sign-extended to 64 bits." so this behaviour should be reliable. > > Also the decrementer would count down from the mask value before we > compare, but that will not toggle the higher order bits and the loop > will work correctly :) > > >> +} >> + >> void init_all_cpus(void) >> { >> struct dt_node *cpus, *cpu; >> unsigned int thread, new_max_pir = 0; >> + int dec_bits = find_dec_bits(); > > You already test function of large decrementer and only then export > ibm,dec-bits > 32. > > You leave LD disabled in LPCR and let the Linux kernel check and > enable. > > Why do we have to test the number of bits again in kernel? Do you > have a reason to believe that test/check in skiboot is not sufficient? > > You have check_large_decrementer() in kernel that will again validate > the number of bits that you have just discovered. I'm just being paranoid. I don't like the idea of having the kernel relying on skiboot to validate architected features. In my mind skiboot is there for handling platform/chip specific details while anything that appears in Books 1-3 should be handled by the kernel. Given that Power is *technically* an open ISA it's conceivable that there might the a non-IBM implementation of ISAv3 that uses a different firmware stack. > > >> cpus = dt_find_by_path(dt_root, "/cpus"); >> assert(cpus); >> @@ -582,6 +627,9 @@ void init_all_cpus(void) >> /* Add associativity properties */ >> add_core_associativity(t); >> >> + /* Add the decrementer width property */ >> + dt_add_property_cells(cpu, "ibm,dec-bits", dec_bits); >> + >> /* Adjust max PIR */ >> if (new_max_pir < (pir + cpu_thread_count - 1)) >> new_max_pir = pir + cpu_thread_count - 1; >> diff --git a/include/processor.h b/include/processor.h >> index 1236c779aac8..48bbf903df58 100644 >> --- a/include/processor.h >> +++ b/include/processor.h >> @@ -96,6 +96,7 @@ >> #define SPR_LPCR_P8_PECE2 PPC_BIT(49) /* Wake on external interrupts */ >> #define SPR_LPCR_P8_PECE3 PPC_BIT(50) /* Wake on decrementer */ >> #define SPR_LPCR_P8_PECE4 PPC_BIT(51) /* Wake on MCs, HMIs, etc... */ >> +#define SPR_LPCR_P9_LD PPC_BIT(46) /* Large decrementer mode bit */ >> >> >> /* Bits in TFMR - control bits */ >> -- >> 2.5.5 >> >> _______________________________________________ >> Skiboot mailing list >> Skiboot@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot >
Oliver O'Halloran <oohall@gmail.com> writes: > ISAv3 adds a mode to increase the size of the decrementer from 32 bits. > The enlarged decrementer can be between 32 and 64 bits wide with the > exact value being implementation dependent. This patch adds support for > detecting the size of the large decrementer and populating each CPU node > with the "ibm,dec-bits" property. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > core/cpu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/processor.h | 1 + > 2 files changed, 49 insertions(+) Thanks! merged to master as of ad53085acc6264b5673fbb8055a3e7feecee6159 I renamed enable_ld() to enable_large_dec() to just not be ambiguous with ld often meaning load and fewer TLAs are always good.
diff --git a/core/cpu.c b/core/cpu.c index ffc264f90907..32b70e0c3713 100644 --- a/core/cpu.c +++ b/core/cpu.c @@ -524,10 +524,55 @@ void init_boot_cpu(void) list_head_init(&global_job_queue); } +static void enable_ld(bool on) +{ + u64 lpcr = mfspr(SPR_LPCR); + + if (on) + lpcr |= SPR_LPCR_P9_LD; + else + lpcr &= ~SPR_LPCR_P9_LD; + + mtspr(SPR_LPCR, lpcr); +} + +#define HIGH_BIT (1ull << 63) + +static int find_dec_bits(void) +{ + int bits = 65; /* we always decrement once */ + u64 mask = ~0ull; + + if (proc_gen < proc_gen_p9) + return 32; + + /* The ISA doesn't specify the width of the decrementer register so we + * need to discover it. When in large mode (LPCR.LD = 1) reads from the + * DEC SPR are sign extended to 64 bits and writes are truncated to the + * physical register width. We can use this behaviour to detect the + * width by starting from an all 1s value and left shifting until we + * read a value from the DEC with it's high bit cleared. + */ + + enable_ld(true); + + do { + bits--; + mask = mask >> 1; + mtspr(SPR_DEC, mask); + } while (mfspr(SPR_DEC) & HIGH_BIT); + + enable_ld(false); + + prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); + return bits; +} + void init_all_cpus(void) { struct dt_node *cpus, *cpu; unsigned int thread, new_max_pir = 0; + int dec_bits = find_dec_bits(); cpus = dt_find_by_path(dt_root, "/cpus"); assert(cpus); @@ -582,6 +627,9 @@ void init_all_cpus(void) /* Add associativity properties */ add_core_associativity(t); + /* Add the decrementer width property */ + dt_add_property_cells(cpu, "ibm,dec-bits", dec_bits); + /* Adjust max PIR */ if (new_max_pir < (pir + cpu_thread_count - 1)) new_max_pir = pir + cpu_thread_count - 1; diff --git a/include/processor.h b/include/processor.h index 1236c779aac8..48bbf903df58 100644 --- a/include/processor.h +++ b/include/processor.h @@ -96,6 +96,7 @@ #define SPR_LPCR_P8_PECE2 PPC_BIT(49) /* Wake on external interrupts */ #define SPR_LPCR_P8_PECE3 PPC_BIT(50) /* Wake on decrementer */ #define SPR_LPCR_P8_PECE4 PPC_BIT(51) /* Wake on MCs, HMIs, etc... */ +#define SPR_LPCR_P9_LD PPC_BIT(46) /* Large decrementer mode bit */ /* Bits in TFMR - control bits */
ISAv3 adds a mode to increase the size of the decrementer from 32 bits. The enlarged decrementer can be between 32 and 64 bits wide with the exact value being implementation dependent. This patch adds support for detecting the size of the large decrementer and populating each CPU node with the "ibm,dec-bits" property. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- core/cpu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/processor.h | 1 + 2 files changed, 49 insertions(+)