[Skiboot] [PATCH 1/5] simplify GET/SETFIELD() use, add MASK_TO_LSH()

[Dan Streetman]

On Tue, Feb 17, 2015 at 4:20 PM, Dan Streetman <ddstreet at us.ibm.com> wrote:
> On Tue, Feb 17, 2015 at 4:09 PM, Dan Streetman <ddstreet at us.ibm.com> wrote:
>> On Tue, Feb 17, 2015 at 3:44 PM, Benjamin Herrenschmidt
>> <benh at kernel.crashing.org> wrote:
>>> On Tue, 2015-02-17 at 15:38 -0500, Dan Streetman wrote:
>>>> Currently, the GETFIELD() and SETFIELD() macros require the user to define
>>>> both the _MASK and the _LSH of the field. However, the shift of the field
>>>> is trivially determinable and there is no reason it needs to be specified
>>>> by the user.
>>>>
>>>> Add MASK_TO_LSH() macro, which determines the shift of a given mask.
>>>>
>>>> Change GETFIELD() and SETFIELD() to use MASK_TO_LSH() to determine the
>>>> shift of the specified mask, instead of requiring it to be specified by
>>>> the user.
>>>
>>> This is quite nice, did you check gcc was resolving it properly at
>>> compile time ?
>>
>> Yep, gcc compiles it fine
>
> To clarify - I did not check the binary output for correctness, I only
> booted the resulting skiboot.lid, which worked as expected.

Re: gcc fully resolving the value, I tried to look at the ppc asm, but
my ppc asm isn't good enough to tell what gcc's doing.  Here is an
example though, in case you can tell (taken with objdump -S):

        ci = instance;
        cfg = SETFIELD(NX_P8_842_CFG_CI, cfg, ci << NX_P8_842_CFG_CI_LSHIFT);
 2c4:   78 a5 13 40     rldicl  r5,r5,2,13
 2c8:   78 a5 f0 02     rotldi  r5,r5,62
 2cc:   64 a5 f8 00     oris    r5,r5,63488
 2d0:   60 a5 00 01     ori     r5,r5,1

        /* Enable all functions */
        cfg = SETFIELD(NX_P8_842_CFG_FC_ENABLE, cfg, CFG_842_FC_ENABLE);

        cfg = SETFIELD(NX_P8_842_CFG_ENABLE, cfg, CFG_842_ENABLE);
 2d4:   7c a5 4b 78     or      r5,r5,r9
 2d8:   f8 a1 00 70     std     r5,112(r1)

        rc = xscom_write(gcid, xcfg, cfg);
 2dc:   48 00 00 01     bl      2dc <.nx_create_842_node+0x2dc>
 2e0:   60 00 00 00     nop
        if (rc)
 2e4:   7c 7b 1b 79     mr.     r27,r3
 2e8:   41 82 00 30     beq     318 <.nx_create_842_node+0x318>
                prerror("NX%d: ERROR: 842 CT %u CI %u config failure %d\n",
 2ec:   3c 82 00 00     addis   r4,r2,0
 2f0:   38 60 00 03     li      r3,3
 2f4:   38 84 00 00     addi    r4,r4,0
 2f8:   7f e5 fb 78     mr      r5,r31
 2fc:   38 c0 00 03     li      r6,3




>
>> and I booted the resulting skiboot.lid fine
>> also.  I'm pretty sure that all gcc versions implement
>> __builtin_ffsl()...probably any other compiler would too, although I
>> assume we really only plan on gcc.
>>
>>>
>>> We could get rid of a whole pile of our _LSH definitions all over the
>>> place...
>>
>> yep, next patch in the set does that, and removes a lot of the _MASK suffixes.
>>
>>>
>>>> Signed-off-by: Dan Streetman <ddstreet at ieee.org>
>>>> ---
>>>>  include/bitutils.h | 11 ++++++-----
>>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/bitutils.h b/include/bitutils.h
>>>> index 7e5b6eb..baa752b 100644
>>>> --- a/include/bitutils.h
>>>> +++ b/include/bitutils.h
>>>> @@ -36,15 +36,16 @@
>>>>   * PPC bitmask field manipulation
>>>>   */
>>>>
>>>> +/* Find left shift from first set bit in mask */
>>>> +#define MASK_TO_LSH(m)               (__builtin_ffsl(m) - 1)
>>>> +
>>>>  /* Extract field fname from val */
>>>> -#define GETFIELD(fname, val)                 \
>>>> -     (((val) & fname##_MASK) >> fname##_LSH)
>>>> +#define GETFIELD(m, v)               (((v) & (m)) >> MASK_TO_LSH(m))
>>>>
>>>>  /* Set field fname of oval to fval
>>>>   * NOTE: oval isn't modified, the combined result is returned
>>>>   */
>>>> -#define SETFIELD(fname, oval, fval)                  \
>>>> -     (((oval) & ~fname##_MASK) | \
>>>> -      ((((typeof(oval))(fval)) << fname##_LSH) & fname##_MASK))
>>>> +#define SETFIELD(m, v, val)                          \
>>>> +     (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
>>>>
>>>>  #endif /* __BITUTILS_H */
>>>
>>>
>>> _______________________________________________
>>> Skiboot mailing list
>>> Skiboot at lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/skiboot