On 5 February 2014 06:23, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.mayd...@linaro.org>
> wrote:
>> + switch (op) {
>> + case 0x05: /* SPSel */
>> + env->pstate = deposit32(env->pstate, 0, 1, imm);
>
> "0","1" hardcoded constants are a bit unfriendly. I guess the current
> macro set doesnt define _SHIFT and _WIDTH definitions, should they be
> added?
>
> FWIW, I have this macro in my tree which makes short work of defining
> mask, shift and width constants as a one liner:
>
> /* Define SHIFT, LENGTH and MASK constants for a field within a register */
>
> #define FIELD(reg, field, length, shift) \
> enum { reg ## _ ## field ## _SHIFT = (shift)}; \
> enum { reg ## _ ## field ## _LENGTH = (length)}; \
> enum { reg ## _ ## field ## _MASK = (((1ULL << (length)) - 1) \
> << (shift)) };
>
> Usage would be something like FIELD(PSTATE, SPSEL, 1, 0)
So, I kind of like this, but I'm a bit reluctant to tie up
this patchset in "add a new generic facility to bitops.h",
and current handling for pstate/cpsr has a lot of hardcoded
constants for bit positions. Is it OK if we do this as a
separate cleanup, or do you feel strongly we should bite
the bullet and do it as part of this series?
thanks
-- PMM
