[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros |
Date: |
Fri, 10 Jun 2016 11:52:44 +0100 |
On 12 May 2016 at 23:46, 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 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?
> 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>
> ---
> 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?
> 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.
> 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.)
> +
> +/* 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
- Re: [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros,
Peter Maydell <=