| Message ID | d99bea7661e6b24148a1fa42a7ed512e2fb7bbe2.1463093051.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@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? > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > [ EI Changes: > * Add Deposit macros > ] > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Signed-off-by: Alistair Francis <alistair.francis@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? > 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
On Fri, Jun 10, 2016 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote: >> From: Peter Crosthwaite <peter.crosthwaite@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.crosthwaite@xilinx.com> >> [ EI Changes: >> * Add Deposit macros >> ] >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> Signed-off-by: Alistair Francis <alistair.francis@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 >
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)) }; + +/* 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