On Sat, Feb 15, 2014 at 2:41 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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?
>
No I think its follow up.
Regards,
Peter
> thanks
> -- PMM
>
