| Message ID | 20190107140428.16388-8-npiggin@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | assorted MCE and SRESET handling and reporting | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
Nicholas Piggin <npiggin@gmail.com> writes: > There is no good reason to "discover" the large decremeter width. > We are firmware, we know the large decrementer width. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Oliver - do you remember the original motivation around detecting it rather than just coding how many bits there were? Was it because we were experimenting with things in sim before the HW design was finalised? Or are we just being fancy? > --- > core/cpu.c | 43 +++++++------------------------------------ > 1 file changed, 7 insertions(+), 36 deletions(-) > > diff --git a/core/cpu.c b/core/cpu.c > index 5d933bf5c..e7ffe3003 100644 > --- a/core/cpu.c > +++ b/core/cpu.c > @@ -1044,48 +1044,19 @@ void init_boot_cpu(void) > init_hid(); > } > > -static void enable_large_dec(bool on) > -{ > - u64 lpcr = mfspr(SPR_LPCR); > - > - if (on) > - lpcr |= SPR_LPCR_P9_LD; > - else > - lpcr &= ~SPR_LPCR_P9_LD; > - > - mtspr(SPR_LPCR, lpcr); > - isync(); > -} > - > -#define HIGH_BIT (1ull << 63) > - > static int find_dec_bits(void) > { > - int bits = 65; /* we always decrement once */ > - u64 mask = ~0ull; > + int bits; > > 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_large_dec(true); > - > - do { > - bits--; > - mask = mask >> 1; > - mtspr(SPR_DEC, mask); > - } while (mfspr(SPR_DEC) & HIGH_BIT); > - > - enable_large_dec(false); > + bits = 32; > + else if (proc_gen == proc_gen_p9) > + bits = 56; > + else > + assert(0); > > prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); > + > return bits; > } > > -- > 2.18.0 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot >
On Mon, Feb 11, 2019 at 3:12 PM Stewart Smith <stewart@linux.ibm.com> wrote: > > Nicholas Piggin <npiggin@gmail.com> writes: > > There is no good reason to "discover" the large decremeter width. > > We are firmware, we know the large decrementer width. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > Oliver - do you remember the original motivation around detecting it > rather than just coding how many bits there were? Was it because we were > experimenting with things in sim before the HW design was finalised? Or > are we just being fancy? Dunno. I'm pretty sure this was one of the first bits of skiboot code I wrote so either, a) I was overthinking it, or b) Mikey told me to. I do remember having to get some mambo bugs fixed to test it though. So it's possible the actual size wasn't finalised yet. Oliver > > > --- > > core/cpu.c | 43 +++++++------------------------------------ > > 1 file changed, 7 insertions(+), 36 deletions(-) > > > > diff --git a/core/cpu.c b/core/cpu.c > > index 5d933bf5c..e7ffe3003 100644 > > --- a/core/cpu.c > > +++ b/core/cpu.c > > @@ -1044,48 +1044,19 @@ void init_boot_cpu(void) > > init_hid(); > > } > > > > -static void enable_large_dec(bool on) > > -{ > > - u64 lpcr = mfspr(SPR_LPCR); > > - > > - if (on) > > - lpcr |= SPR_LPCR_P9_LD; > > - else > > - lpcr &= ~SPR_LPCR_P9_LD; > > - > > - mtspr(SPR_LPCR, lpcr); > > - isync(); > > -} > > - > > -#define HIGH_BIT (1ull << 63) > > - > > static int find_dec_bits(void) > > { > > - int bits = 65; /* we always decrement once */ > > - u64 mask = ~0ull; > > + int bits; > > > > 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_large_dec(true); > > - > > - do { > > - bits--; > > - mask = mask >> 1; > > - mtspr(SPR_DEC, mask); > > - } while (mfspr(SPR_DEC) & HIGH_BIT); > > - > > - enable_large_dec(false); > > + bits = 32; > > + else if (proc_gen == proc_gen_p9) > > + bits = 56; > > + else > > + assert(0); > > > > prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); > > + > > return bits; > > } > > > > -- > > 2.18.0 > > > > _______________________________________________ > > Skiboot mailing list > > Skiboot@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/skiboot > > > > -- > Stewart Smith > OPAL Architect, IBM. >
Oliver <oohall@gmail.com> writes: > On Mon, Feb 11, 2019 at 3:12 PM Stewart Smith <stewart@linux.ibm.com> wrote: >> >> Nicholas Piggin <npiggin@gmail.com> writes: >> > There is no good reason to "discover" the large decremeter width. >> > We are firmware, we know the large decrementer width. >> > >> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> >> Oliver - do you remember the original motivation around detecting it >> rather than just coding how many bits there were? Was it because we were >> experimenting with things in sim before the HW design was finalised? Or >> are we just being fancy? > > Dunno. I'm pretty sure this was one of the first bits of skiboot code > I wrote so either, > > a) I was overthinking it, or > b) Mikey told me to. > > I do remember having to get some mambo bugs fixed to test it though. > So it's possible the actual size wasn't finalised yet. Mikey, was it (b) ? (and at this point am *I* overthinking this? Hard-coding it is *fine*) > > Oliver > >> >> > --- >> > core/cpu.c | 43 +++++++------------------------------------ >> > 1 file changed, 7 insertions(+), 36 deletions(-) >> > >> > diff --git a/core/cpu.c b/core/cpu.c >> > index 5d933bf5c..e7ffe3003 100644 >> > --- a/core/cpu.c >> > +++ b/core/cpu.c >> > @@ -1044,48 +1044,19 @@ void init_boot_cpu(void) >> > init_hid(); >> > } >> > >> > -static void enable_large_dec(bool on) >> > -{ >> > - u64 lpcr = mfspr(SPR_LPCR); >> > - >> > - if (on) >> > - lpcr |= SPR_LPCR_P9_LD; >> > - else >> > - lpcr &= ~SPR_LPCR_P9_LD; >> > - >> > - mtspr(SPR_LPCR, lpcr); >> > - isync(); >> > -} >> > - >> > -#define HIGH_BIT (1ull << 63) >> > - >> > static int find_dec_bits(void) >> > { >> > - int bits = 65; /* we always decrement once */ >> > - u64 mask = ~0ull; >> > + int bits; >> > >> > 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_large_dec(true); >> > - >> > - do { >> > - bits--; >> > - mask = mask >> 1; >> > - mtspr(SPR_DEC, mask); >> > - } while (mfspr(SPR_DEC) & HIGH_BIT); >> > - >> > - enable_large_dec(false); >> > + bits = 32; >> > + else if (proc_gen == proc_gen_p9) >> > + bits = 56; >> > + else >> > + assert(0); >> > >> > prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); >> > + >> > return bits; >> > } >> > >> > -- >> > 2.18.0 >> > >> > _______________________________________________ >> > Skiboot mailing list >> > Skiboot@lists.ozlabs.org >> > https://lists.ozlabs.org/listinfo/skiboot >> > >> >> -- >> Stewart Smith >> OPAL Architect, IBM. >> >
On Tue, 2019-02-12 at 12:23 +1100, Stewart Smith wrote: > Oliver <oohall@gmail.com> writes: > > On Mon, Feb 11, 2019 at 3:12 PM Stewart Smith <stewart@linux.ibm.com> wrote: > > > Nicholas Piggin <npiggin@gmail.com> writes: > > > > There is no good reason to "discover" the large decremeter width. > > > > We are firmware, we know the large decrementer width. > > > > > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > > > > > Oliver - do you remember the original motivation around detecting it > > > rather than just coding how many bits there were? Was it because we were > > > experimenting with things in sim before the HW design was finalised? Or > > > are we just being fancy? > > > > Dunno. I'm pretty sure this was one of the first bits of skiboot code > > I wrote so either, > > > > a) I was overthinking it, or > > b) Mikey told me to. > > > > I do remember having to get some mambo bugs fixed to test it though. > > So it's possible the actual size wasn't finalised yet. > > Mikey, was it (b) ? > > (and at this point am *I* overthinking this? Hard-coding it is *fine*) IIRC, we wanted to make it future proof and it was easy enough to do, so we did. Is there a problem with auto detecting it? Mikey
Michael Neuling <mikey@neuling.org> writes: > On Tue, 2019-02-12 at 12:23 +1100, Stewart Smith wrote: >> Oliver <oohall@gmail.com> writes: >> > On Mon, Feb 11, 2019 at 3:12 PM Stewart Smith <stewart@linux.ibm.com> wrote: >> > > Nicholas Piggin <npiggin@gmail.com> writes: >> > > > There is no good reason to "discover" the large decremeter width. >> > > > We are firmware, we know the large decrementer width. >> > > > >> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> > > >> > > Oliver - do you remember the original motivation around detecting it >> > > rather than just coding how many bits there were? Was it because we were >> > > experimenting with things in sim before the HW design was finalised? Or >> > > are we just being fancy? >> > >> > Dunno. I'm pretty sure this was one of the first bits of skiboot code >> > I wrote so either, >> > >> > a) I was overthinking it, or >> > b) Mikey told me to. >> > >> > I do remember having to get some mambo bugs fixed to test it though. >> > So it's possible the actual size wasn't finalised yet. >> >> Mikey, was it (b) ? >> >> (and at this point am *I* overthinking this? Hard-coding it is *fine*) > > IIRC, we wanted to make it future proof and it was easy enough to do, so we did. > > Is there a problem with auto detecting it? Not really? I'll drop this patch for the time being.
Stewart Smith's on February 13, 2019 1:38 pm: > Michael Neuling <mikey@neuling.org> writes: >> On Tue, 2019-02-12 at 12:23 +1100, Stewart Smith wrote: >>> Oliver <oohall@gmail.com> writes: >>> > On Mon, Feb 11, 2019 at 3:12 PM Stewart Smith <stewart@linux.ibm.com> wrote: >>> > > Nicholas Piggin <npiggin@gmail.com> writes: >>> > > > There is no good reason to "discover" the large decremeter width. >>> > > > We are firmware, we know the large decrementer width. >>> > > > >>> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> > > >>> > > Oliver - do you remember the original motivation around detecting it >>> > > rather than just coding how many bits there were? Was it because we were >>> > > experimenting with things in sim before the HW design was finalised? Or >>> > > are we just being fancy? >>> > >>> > Dunno. I'm pretty sure this was one of the first bits of skiboot code >>> > I wrote so either, >>> > >>> > a) I was overthinking it, or >>> > b) Mikey told me to. >>> > >>> > I do remember having to get some mambo bugs fixed to test it though. >>> > So it's possible the actual size wasn't finalised yet. >>> >>> Mikey, was it (b) ? >>> >>> (and at this point am *I* overthinking this? Hard-coding it is *fine*) >> >> IIRC, we wanted to make it future proof and it was easy enough to do, so we did. >> >> Is there a problem with auto detecting it? > > Not really? I'll drop this patch for the time being. Ah, I ran into a problem when running a later patch that set a decrementer timer ticking during boot, bad things happened here, fixed with this patch. That's not a serious patch yet, so this can wait. I'm generally in favour of hard coding things when we know them, but presumably this could also be fixed by disabling EE over the detection if people want to keep the detection. Thanks, Nick
diff --git a/core/cpu.c b/core/cpu.c index 5d933bf5c..e7ffe3003 100644 --- a/core/cpu.c +++ b/core/cpu.c @@ -1044,48 +1044,19 @@ void init_boot_cpu(void) init_hid(); } -static void enable_large_dec(bool on) -{ - u64 lpcr = mfspr(SPR_LPCR); - - if (on) - lpcr |= SPR_LPCR_P9_LD; - else - lpcr &= ~SPR_LPCR_P9_LD; - - mtspr(SPR_LPCR, lpcr); - isync(); -} - -#define HIGH_BIT (1ull << 63) - static int find_dec_bits(void) { - int bits = 65; /* we always decrement once */ - u64 mask = ~0ull; + int bits; 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_large_dec(true); - - do { - bits--; - mask = mask >> 1; - mtspr(SPR_DEC, mask); - } while (mfspr(SPR_DEC) & HIGH_BIT); - - enable_large_dec(false); + bits = 32; + else if (proc_gen == proc_gen_p9) + bits = 56; + else + assert(0); prlog(PR_DEBUG, "CPU: decrementer bits %d\n", bits); + return bits; }
There is no good reason to "discover" the large decremeter width. We are firmware, we know the large decrementer width. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- core/cpu.c | 43 +++++++------------------------------------ 1 file changed, 7 insertions(+), 36 deletions(-)