[Skiboot] [PATCH] cpu: supply ibm,dec-bits via devicetree

[oliver]

On Tue, Jun 28, 2016 at 1:31 AM, Vaidyanathan Srinivasan
<svaidy at linux.vnet.ibm.com> wrote:
> * Oliver O'Halloran <oohall at 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 at gmail.com>
>
> Reviewed-by: Vaidyanathan Srinivasan <svaidy at 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 at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>