Re: [Qemu-devel] [PATCH v7 04/12] register: Define REG and FIELD macros

[Alistair Francis]

On Thu, Jun 23, 2016 at 5:39 AM, Peter Maydell <address@hidden> wrote:
> On 22 June 2016 at 21:23, Alistair Francis <address@hidden> wrote:
>> From: Peter Crosthwaite <address@hidden>
>>
>> Define some macros that can be used for defining registers and fields.
>>
>> The REG32 macro will define A_FOO, for the byte address of a register
>> as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
>>
>> The FIELD macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and
>> FOO_BAR_LENGTH constants for field BAR in register FOO.
>>
>> Finally, there are some shorthand helpers for extracting/depositing
>> fields from registers based on these naming schemes.
>>
>> Usage can greatly reduce the verbosity of device code.
>>
>> The deposit and extract macros (eg FIELD_EX32, FIELD_DP32  etc.) can be
>> used to generate extract and deposits without any repetition of the name
>> stems.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> [ EI Changes:
>>   * Add Deposit macros
>> ]
>> Signed-off-by: Edgar E. Iglesias <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index e160150..216b679 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -150,4 +150,47 @@ void register_write_memory(void *opaque, hwaddr addr, 
>> uint64_t value,
>>
>>  uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
>>
>> +/* Define constants for a 32 bit register */
>> +
>> +/* This macro will define A_FOO, for the byte address of a register
>> + * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
>> + */
>> +#define REG32(reg, addr)                                                  \
>> +    enum { A_ ## reg = (addr) };                                          \
>> +    enum { R_ ## reg = (addr) / 4 };
>> +
>> +/* Define SHIFT, LEGTH and MASK constants for a field within a register */
>> +
>
> "LENGTH".

Fixed

>
>> +/* Deposit a register field.  */
>> +#define FIELD_DP32(storage, reg, field, val) ({                           \
>> +    struct {                                                              \
>> +        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
>> +    } v = { .v = val };                                                   \
>> +    uint32_t d;                                                           \
>> +    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
>> +                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
>> +    d; })
>
> If you insist on different semantics to deposit32() (which I
> still think is a bad idea), can we at least have a comment
> noting the difference?

I have added a comment.

>
> (Do you get a warning for putting a signed negative value into
> a field?)

No, I don't see any warnings for forcing negative values in.

Thanks,

Alistair

>
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
>
> thanks
> -- PMM
>