On Fri, Jun 10, 2016 at 3:52 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.fran...@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
>>
>> 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 F_EX32, AF_DP32 etc.) can be used
>> to generate extract and deposits without any repetition of the name
>> stems.
>
> Could we have the documentation of what these macros do in the code,
> not just in the commit message and the extra remarks, please?
Fixed.
>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
>> [ EI Changes:
>> * Add Deposit macros
>> ]
>> Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
>> ---
>> E.g. Currently you have to define something like:
>>
>> \#define R_FOOREG (0x84/4)
>> \#define R_FOOREG_BARFIELD_SHIFT 10
>> \#define R_FOOREG_BARFIELD_LENGTH 5
>>
>> uint32_t foobar_val = extract32(s->regs[R_FOOREG],
>> R_FOOREG_BARFIELD_SHIFT,
>> R_FOOREG_BARFIELD_LENGTH);
>>
>> Which has:
>> 2 macro definitions per field
>> 3 register names ("FOOREG") per extract
>> 2 field names ("BARFIELD") per extract
>>
>> With these macros this becomes:
>>
>> REG32(FOOREG, 0x84)
>> FIELD(FOOREG, BARFIELD, 10, 5)
>>
>> uint32_t foobar_val = AF_EX32(s->regs, FOOREG, BARFIELD)
>>
>> Which has:
>> 1 macro definition per field
>> 1 register name per extract
>> 1 field name per extract
>>
>> If you are not using arrays for the register data you can just use the
>> non-array "F_" variants and still save 2 name stems:
>>
>> uint32_t foobar_val = F_EX32(s->fooreg, FOOREG, BARFIELD)
>>
>> Deposit is similar for depositing values. Deposit has compile-time
>> overflow checking for literals.
>> For example:
>>
>> REG32(XYZ1, 0x84)
>> FIELD(XYZ1, TRC, 0, 4)
>>
>> /* Correctly set XYZ1.TRC = 5. */
>> AF_DP32(s->regs, XYZ1, TRC, 5);
>>
>> /* Incorrectly set XYZ1.TRC = 16. */
>> AF_DP32(s->regs, XYZ1, TRC, 16);
>
> These deposit functions seem a bit too cryptically named to me;
> can we come up with something a bit less abbreviated?
Ok, I have changed the names to be longer.
>
>> The latter assignment results in:
>> warning: large integer implicitly truncated to unsigned type [-Woverflow]
>
> This is inconsistent with the behaviour of deposit32() and
> deposit64() which are documented to ignore oversized values.
Should we really just ignore this? This seems like a possible common
mistake and I think it is a good idea to warn against it.
>
>> include/hw/register.h | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 786707b..e0aac91 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -157,4 +157,42 @@ void register_write_memory_le(void *opaque, hwaddr
>> addr, uint64_t value,
>> uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
>> uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
>>
>> +/* Define constants for a 32 bit register */
>> +#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 */
>> +#define FIELD(reg, field, shift, length) \
>> + enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)}; \
>> + enum { R_ ## reg ## _ ## field ## _LENGTH = (length)}; \
>> + enum { R_ ## reg ## _ ## field ## _MASK = (((1ULL << (length)) - 1) \
>> + << (shift)) };
>
> We defined a MAKE_64BIT_MASK macro in patch 1, so we can use it here.
> (Also this open-coded version has the same "undefined behaviour if
> length is 64" issue.)
Fixed.
Thanks,
Alistair
>
>> +
>> +/* Extract a field from a register */
>> +
>> +#define F_EX32(storage, reg, field) \
>> + extract32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>> + R_ ## reg ## _ ## field ## _LENGTH)
>> +
>> +/* Extract a field from an array of registers */
>> +
>> +#define AF_EX32(regs, reg, field) \
>> + F_EX32((regs)[R_ ## reg], reg, field)
>> +
>> +/* Deposit a register field. */
>> +
>> +#define F_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; })
>> +
>> +/* Deposit a field to array of registers. */
>> +
>> +#define AF_DP32(regs, reg, field, val) \
>> + (regs)[R_ ## reg] = F_DP32((regs)[R_ ## reg], reg, field, val);
>> #endif
>> --
>> 2.7.4
>>
>
> thanks
> -- PMM
>
