Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, start, end) SET_BITFIELDS(addr, name, value, [name2, value2, ...])
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits(®, 0x7 << 24, 0x2 << 24);
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/1
diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index e40b40f..b3b51c2 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -53,4 +53,82 @@ } #endif /* !__ROMCC__ */
+/* + * Utilities to help processing bit fields. + * + * To define a bit field (usually inside a register, do: + * + * DEFINE_BITFIELD(name, start end) + * + * - name: String for name of the field to access. + * - start: starting (high) bit from data sheet. + * - end: ending (low) bit from data sheet. + * + * Then, set the values using: + * + * SET_BITFIELDS(®, name, value, [name, value, ...]) + * + * Example: + * + * DEFINE_BITFIELD(DISP_TYPE, 2, 1); + * DEFINE_BITFIELD(DISP_EN, 0, 0); + * + * SET_BITFIELDS(&disp_regs.ctrl, DISP_TYPE, 2); + * SET_BITFIELDS(&disp_regs.ctrl, DISP_EN, 0); + * + * SET_BITFIELDS(&disp_regs.ctrl, DISP_TYPE, 1, DISP_EN, 1); + * + * This will be translated to: + * + * clrsetbits_le32(&disp_regs.ctrl, 0x6, 0x4); + * clrsetbits_le32(&disp_regs.ctrl, 0x1, 0x0); + * + * clrsetbits_le32(&disp_regs.ctrl, 0x7, 0x3); + */ + +#define DEFINE_BITFIELD(name, start, end) \ + _Static_assert(start >= end, "invalid bit field range"); \ + enum { \ + name##_FIELD_SHIFT = (end), \ + name##_FIELD_SIZE = (start) - (end) + 1 } + +#define _BF_MASK(name, value) \ + (((1 << name##_FIELD_SIZE) - 1) << name##_FIELD_SHIFT) + +#define _BF_VALUE(name, value) \ + ((value) << name##_FIELD_SHIFT) + +#define _BF_APPLY1(op, name, value, ...) (op(name, value)) +#define _BF_APPLY2(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY1(op, __VA_ARGS__)) +#define _BF_APPLY3(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY2(op, __VA_ARGS__)) +#define _BF_APPLY4(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY3(op, __VA_ARGS__)) +#define _BF_APPLY5(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY4(op, __VA_ARGS__)) +#define _BF_APPLY6(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY5(op, __VA_ARGS__)) +#define _BF_APPLY7(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY6(op, __VA_ARGS__)) +#define _BF_APPLY8(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY7(op, __VA_ARGS__)) +#define _BF_APPLYINVALID(...) \ + _Static_assert(0, "Invalid arguments to SET_BITFIELDS"); + +#define _SET_BITFIELDS_IMPL(addr, \ + n1, v1, n2, v2, n3, v3, n4, v4, n5, v5, n6, v6, n7, v7, n8, v8, \ + NARGS, ...) \ + \ + clrsetbits_le32(addr, \ + _BF_APPLY##NARGS(_BF_MASK, n1, v1, n2, v2, n3, v3, n4, v4, \ + n5, v5, n6, v6, n7, v7, n8, v8), \ + _BF_APPLY##NARGS(_BF_VALUE, n1, v1, n2, v2, n3, v3, n4, v4, \ + n5, v5, n6, v6, n7, v7, n8, v8)) + +#define SET_BITFIELDS(addr, ...) \ + _SET_BITFIELDS_IMPL(addr, __VA_ARGS__, \ + 8, INVALID, 7, INVALID, 6, INVALID, 5, INVALID, \ + 4, INVALID, 3, INVALID, 2, INVALID, 1, INVALID) + #endif /* __DEVICE_MMIO_H__ */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h@8... PS1, Line 89: #define DEFINE_BITFIELD(name, start, end) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h@1... PS1, Line 116: #define _BF_APPLYINVALID(...) \ macros should not use a trailing semicolon
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#2).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, start, end) SET_BITFIELDS(addr, name, value, [name2, value2, ...])
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits(®, 0x7 << 24, 0x2 << 24);
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/2/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/2/src/include/device/mmio.h@9... PS2, Line 92: #define DEFINE_BITFIELD(name, start, end) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/35463/2/src/include/device/mmio.h@1... PS2, Line 119: #define _BF_APPLYINVALID(...) \ macros should not use a trailing semicolon
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#3).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, start, end) SET_BITFIELDS(addr, name, value, [name2, value2, ...])
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits(®, 0x7 << 24, 0x2 << 24);
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h@9... PS3, Line 92: #define DEFINE_BITFIELD(name, start, end) \ Macros with multiple statements should be enclosed in a do - while loop
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h@8... PS1, Line 89: #define DEFINE_BITFIELD(name, start, end) \
Macros with multiple statements should be enclosed in a do - while loop
we probably won't fix this because we do need range checking, but enum also need to be declared outside do-while block.
https://review.coreboot.org/c/coreboot/+/35463/1/src/include/device/mmio.h@1... PS1, Line 116: #define _BF_APPLYINVALID(...) \
macros should not use a trailing semicolon
Done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h@5... PS3, Line 59: usually inside a register Missing )
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h@6... PS3, Line 61: * DEFINE_BITFIELD(name, start, end); I somehow think "start >= end" is a little bit counter-intuitive. Maybe "high" & "low" are more self-explanatory.
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#4).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, start, end) SET_BITFIELDS(addr, name, value, [name2, value2, ...])
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits(®, 0x7 << 24, 0x2 << 24);
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/4
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h@5... PS3, Line 59: usually inside a register
Missing )
Done
https://review.coreboot.org/c/coreboot/+/35463/3/src/include/device/mmio.h@6... PS3, Line 61: * DEFINE_BITFIELD(name, start, end);
I somehow think "start >= end" is a little bit counter-intuitive. […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/4/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/4/src/include/device/mmio.h@9... PS4, Line 92: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#5).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, start, end) SET_BITFIELDS(addr, name, value, [name2, value2, ...]) GET_BITFIELD(value, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
GET_BITFIELD(readl(®), SEC_VIO)
That is equivalent to:
(readl(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 84 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@9... PS5, Line 92: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35463/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35463/5//COMMIT_MSG@24 PS5, Line 24: DEFINE_BITFIELD(name, start, end) nit: should also say (name, high, low), like the code does now
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@5... PS5, Line 54: #endif /* !__ROMCC__ */ These won't work in ROMCC anyway, so even though they're just macros might as well put them within the !ROMCC block.
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@6... PS5, Line 63: * - name: String for name of the field to access. nit: Just "Name of the field to access." (it's not a string, just a preprocessor token)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@6... PS5, Line 64: * - high_bit: starting (high) bit from data sheet. nit: maybe say "highest bit that's part of the bit field" to make extra clear that it is inclusive (on both ends)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \ I see why you'd want it this way (rather than put the read32() in the macro), but I don't really like the disparity of GET operating on variables and SET doing MMIO writes. Maybe rename SET_BITFIELDS to WRITE32_BITFIELDS instead? (Could also consider adding a SET_BITFIELDS that operates on a memory value and a READ32_BITFIELD that does a read32() then.)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35463/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35463/5//COMMIT_MSG@24 PS5, Line 24: DEFINE_BITFIELD(name, start, end)
nit: should also say (name, high, low), like the code does now
Done
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@5... PS5, Line 54: #endif /* !__ROMCC__ */
These won't work in ROMCC anyway, so even though they're just macros might as well put them within t […]
Done
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@6... PS5, Line 63: * - name: String for name of the field to access.
nit: Just "Name of the field to access. […]
Done
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@6... PS5, Line 64: * - high_bit: starting (high) bit from data sheet.
nit: maybe say "highest bit that's part of the bit field" to make extra clear that it is inclusive ( […]
Done
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \
I see why you'd want it this way (rather than put the read32() in the macro), but I don't really lik […]
I thought about write, but it's really more like "setting some of the fields" and close to clrsetbits_le32 (and is shorter).
What if I just rename this to EXTRACT_BITFIELD?
In the mean time I can add GET_BITFIELD by read32.
Well, maybe SET32_BITFIELDS GET32_BITFIELD
EXTRACT_BITFIELD (it does not really have length restriction)
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#6).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) SET32_BITFIELDS(addr, name, value, [name2, value2, ...]) GET32_BITFIELD(addr, name) EXTRACT_BITFIELD(value, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET32_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
GET32_BITFIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/6/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/6/src/include/device/mmio.h@9... PS6, Line 91: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#7).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) SET32_BITFIELDS(addr, name, value, [name2, value2, ...]) GET32_BITFIELD(addr, name) EXTRACT_BITFIELD(value, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
SET32_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
GET32_BITFIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/7/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/7/src/include/device/mmio.h@9... PS7, Line 91: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 7:
Or do you think it'll be better if we name them
WRITE32_BF READ32_BF
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 7:
or WRITE32_FIELDS() READ32_FIELD()
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#8).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_FIELDS(addr, name, value, [name2, value2, ...]) READ32_FIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_FIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
REAED32_FIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/8/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/8/src/include/device/mmio.h@9... PS8, Line 91: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \
I thought about write, but it's really more like "setting some of the fields" and close to clrsetbi […]
Done with WRITE32_FIELDS and READ32_FIELD.
And to get multiple fields from one reg3, do read32 then EXTRACT_BITFIELD multiple times.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \
Done with WRITE32_FIELDS and READ32_FIELD. […]
Hmm... the SET32/GET32/EXTRACT also makes a lot of sense to me and is shorter. But sounds like you changed your mind now?
One more thing I thought of now is that sometimes you know you don't need to preserve any data (e.g. because you're initializing a controller you just reset), so you don't need to do a full read-modify-write... but you're still writing fields in the end, so using this API would still be nice. Do we maybe want to cover that case as well? Maybe use SET32 for the read-modify-write and WRITE32 for a simple write to the whole register (then the macro names are also closer to the MMIO primitives they're based on)?
I'm okay if you want to merge it like this, but I think going to the SET32/GET32 model (with potentially also a WRITE32 option) would also be a good option. I'll leave it up to you.
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#9).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_FIELDS(addr, name, value, [name2, value2, ...]) READ32_FIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_FIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
REAED32_FIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 102: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \
Hmm... the SET32/GET32/EXTRACT also makes a lot of sense to me and is shorter. […]
Good idea. Done.
GET32 is actually READ32.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 9: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@6... PS9, Line 60: DEFINE_BITFIELD nit: why does this and EXTRACT say BITFIELD(S) when all the others just say FIELD(s)? I'd standardize on FIELD(S) for all of them.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@6... PS9, Line 68: * SET32_FIELDS(®, name, value, [name, value, ...]) nit: might be worth adding an explicit sentence explaining what it does to each of these (especially for the SET32/WRITE32 difference).
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@7... PS9, Line 78: * SET32_FIELDS(&disp_regs.ctrl, DISP_TYPE, 2); nit: weird indentation?
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@9... PS9, Line 99: * setting up to 8 fields at one invocation. nit: technically, SET32 is LE32 and WRITE32 uses CPU byte order (which is a bit of an artifact of the fact that we only have clrsetbits with explicit byte orders, I think it would make more sense if we just used a clrsetbits32() that used CPU byte order most of the time... but as long as we don't have BE CPUs, and we likely never will, none of this really matters).
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 106: name##_FIELD_SIZE = (high_bit) - (low_bit) + 1 } nit: for existing macros that you're supposed to call completely out of normal C code context (e.g. the memlayout stuff, or DECLARE_REGION), you're not supposed to call them with semicolons at the end. I think that would probably make sense here as well, so I'd consider putting the semicolon in the macro (although you normally don't do that for stuff that's meant to be used like a C statement... but this isn't).
Also, for optical balancing I'd put the closing brace on a new line.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 158: 4, INVALID, 3, INVALID, 2, INVALID, 1, INVALID) nit: could deduplicate a few more intermediary macros with something like
#define _BF_IMPL2(op, addr, n1, v1, <...etc...>) op(addr, _BF_APPLY##NARGS(_BF_MASK, n1, v1, <...etc...>), _BF_APPLY##NARGS(_BF_VALUE, n1, v1, <...etc...>)) #define _BF_IMPL(op, addr, ...) _BF_IMPL2(op, addr, __VA_ARGS__, 8, INVALID, <...etc...>) #define _WRITE32_FIELDS_IMPL(addr, masks, values) write32(addr, values) #define WRITE32_FIELDS(addr, ...) _BF_IMPL(_WRITE32_FIELDS_IMPL, addr, __VA_ARGS__)
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#10).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_FIELDS(addr, name, value, [name2, value2, ...]) READ32_FIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_FIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
REAED32_FIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 120 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/10
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@6... PS9, Line 60: DEFINE_BITFIELD
nit: why does this and EXTRACT say BITFIELD(S) when all the others just say FIELD(s)? I'd standardiz […]
I feel for definition, it was more clear to call it BITFIELD; and for set/write/read, it's clear enough we're dealing with fields in regs.
But if you think FIELD is still clear enough, then yes we may unify them.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@7... PS9, Line 78: * SET32_FIELDS(&disp_regs.ctrl, DISP_TYPE, 2);
nit: weird indentation?
Done
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@9... PS9, Line 99: * setting up to 8 fields at one invocation.
nit: technically, SET32 is LE32 and WRITE32 uses CPU byte order (which is a bit of an artifact of th […]
well there's currently no clrsetbits32.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 106: name##_FIELD_SIZE = (high_bit) - (low_bit) + 1 }
nit: for existing macros that you're supposed to call completely out of normal C code context (e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 158: 4, INVALID, 3, INVALID, 2, INVALID, 1, INVALID)
nit: could deduplicate a few more intermediary macros with something like […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 10:
BTW, I just found that 3rdparty/chromeec/include/sfdp.h has similar definition:
SFDP_DEFINE_BITFIELD(SFDP_HEADER_DW1_P, 31, 24);
They also don't put semicolon.
So will it be more clear if we go back to always say BITFIELD?
DEFINE_BITFIELD EXTRACT_BITFIELD
SET32_BITFIELDS WRITE32_BITFIELDS READ32_BITFIELDS
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#11).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_BITFIELDS(addr, name, value, [name2, value2, ...]) READ32_BITFIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
REAED32_BITFIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 120 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h@... PS11, Line 114: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h@... PS11, Line 114: #define DEFINE_BITFIELD(name, high_bit, low_bit) \ macros should not use a trailing semicolon
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35463/11//COMMIT_MSG@48 PS11, Line 48: REAED32_BITFIELD(®, SEC_VIO) READ32_BITFIELD
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h@... PS11, Line 112: */ Should that also into the coding style documentation or something related, new developers can easily read?
Hello Yu-Ping Wu, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35463
to look at the new patch set (#12).
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_BITFIELDS(addr, name, value, [name2, value2, ...]) READ32_BITFIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
READ32_BITFIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/include/device/mmio.h 1 file changed, 120 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/35463/12
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35463/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35463/11//COMMIT_MSG@48 PS11, Line 48: REAED32_BITFIELD(®, SEC_VIO)
READ32_BITFIELD
Done
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/11/src/include/device/mmio.h@... PS11, Line 112: */
Should that also into the coding style documentation or something related, new developers can easily […]
we can do that later if we find these macro being really helpful, and have few existing programs migrated to using that.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
device/mmio.h: Add bit field helpers
When accessing register with multiple bit fields, the common approach is to use clrsetbits_le32, for example:
clrsetbits(®, (1 << 0) | (0x3 << 1) | (0x7 << 10), (1 << 0) | (0x1 << 1) | (0x5 << 10));
This hard to maintain because we have to calculate the mask values manually, make sure the duplicated shift (offset) was set correctly. And it may be even worse if the value to set will be based on some runtime values (that many developers will do a if-block with two very similar argument list), and leaving lots of magic numbers.
We want to encourage developers always giving field names, and have a better way of setting fields. The proposed utility macros are:
DEFINE_BITFIELD(name, high_bit, low_bit) EXTRACT_BITFIELD(value, name) WRITE32_BITFIELDS(addr, name, value, [name2, value2, ...]) READ32_BITFIELD(addr, name)
Where a developer can easily convert from data sheet like
BITS NAME 26:24 SEC_VIO
Into a declaration
DEFINE_BITFIELD(SEC_VIO, 26, 24)
Then, a simple call can set the field as:
WRITE32_BITFIELDS(®, SEC_VIO, 2);
That is much easier to understand than
clrsetbits_le32(®, 0x7 << 24, 0x2 << 24);
And to extract the value:
READ32_BITFIELD(®, SEC_VIO)
That is equivalent to:
(read32(®) & 0x3) >> 24
Change-Id: I8a1b17142f7a7dc6c441b0b1ee67d60d73ec8cc8 Signed-off-by: Hung-Te Lin hungte@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/35463 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/device/mmio.h 1 file changed, 120 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index e40b40f..7b95a3c 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -51,6 +51,126 @@ buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo, fifo_stride, fifo_width); } + +/* + * Utilities to help processing bit fields. + * + * To define a bit field (usually inside a register), do: + * + * DEFINE_BITFIELD(name, high_bit, low_bit) + * + * - name: Name of the field to access. + * - high_bit: highest bit that's part of the bit field. + * - low_bit: lowest bit in the bit field. + * + * To extract one field value from a raw reg value: + * + * EXTRACT_BITFIELD(value, name); + * + * To read from an MMIO register and extract one field from it: + * + * READ32_BITFIELD(®, name); + * + * To write into an MMIO register, set given fields (by names) to specified + * values, and all other bits to zero (usually used for resetting a register): + * + * WRITE32_BITFIELDS(®, name, value, [name, value, ...]) + * + * To write into an MMIO register, set given fields (by names) to specified + * values, and leaving all others "unchanged" (usually used for updating some + * settings): + * + * SET32_BITFIELDS(®, name, value, [name, value, ...]) + * + * Examples: + * + * DEFINE_BITFIELD(DISP_TYPE, 2, 1) + * DEFINE_BITFIELD(DISP_EN, 0, 0) + * + * SET32_BITFIELDS(&disp_regs.ctrl, DISP_TYPE, 2); + * SET32_BITFIELDS(&disp_regs.ctrl, DISP_EN, 0); + * + * SET32_BITFIELDS(&disp_regs.ctrl, DISP_TYPE, 1, DISP_EN, 1); + * WRITE32_BITFIELDS(&disp_regs.ctrl, DISP_TYPE, 1, DISP_EN, 1); + * + * READ32_BITFIELD(®, DISP_TYPE) + * EXTRACT_BITFIELD(value, DISP_TYPE) + * + * These will be translated to: + * + * clrsetbits_le32(&disp_regs.ctrl, 0x6, 0x4); + * clrsetbits_le32(&disp_regs.ctrl, 0x1, 0x0); + * + * clrsetbits_le32(&disp_regs.ctrl, 0x7, 0x3); + * write32(&disp_regs.ctrl, 0x3); + * + * (read32(®) & 0x6) >> 1 + * (value & 0x6) >> 1 + * + * The {WRITE,SET}32_BITFIELDS currently only allows setting up to 8 fields at + * one invocation. + */ + +#define DEFINE_BITFIELD(name, high_bit, low_bit) \ + _Static_assert(high_bit >= low_bit, "invalid bit field range"); \ + enum { \ + name##_BITFIELD_SHIFT = (low_bit), \ + name##_BITFIELD_SIZE = (high_bit) - (low_bit) + 1, \ + }; + +#define _BF_MASK(name, value) \ + (((1 << name##_BITFIELD_SIZE) - 1) << name##_BITFIELD_SHIFT) + +#define _BF_VALUE(name, value) \ + ((value) << name##_BITFIELD_SHIFT) + +#define _BF_APPLY1(op, name, value, ...) (op(name, value)) +#define _BF_APPLY2(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY1(op, __VA_ARGS__)) +#define _BF_APPLY3(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY2(op, __VA_ARGS__)) +#define _BF_APPLY4(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY3(op, __VA_ARGS__)) +#define _BF_APPLY5(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY4(op, __VA_ARGS__)) +#define _BF_APPLY6(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY5(op, __VA_ARGS__)) +#define _BF_APPLY7(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY6(op, __VA_ARGS__)) +#define _BF_APPLY8(op, name, value, ...) ((op(name, value)) | \ + _BF_APPLY7(op, __VA_ARGS__)) +#define _BF_APPLYINVALID(...) \ + _Static_assert(0, "Invalid arguments for {WRITE,SET}*_BITFIELDS") + +#define _BF_IMPL2(op, addr, \ + n1, v1, n2, v2, n3, v3, n4, v4, n5, v5, n6, v6, n7, v7, n8, v8, \ + NARGS, ...) \ + \ + op(addr, \ + _BF_APPLY##NARGS(_BF_MASK, n1, v1, n2, v2, n3, v3, n4, v4, \ + n5, v5, n6, v6, n7, v7, n8, v8), \ + _BF_APPLY##NARGS(_BF_VALUE, n1, v1, n2, v2, n3, v3, n4, v4, \ + n5, v5, n6, v6, n7, v7, n8, v8)) + +#define _BF_IMPL(op, addr, ...) \ + _BF_IMPL2(op, addr, __VA_ARGS__, \ + 8, INVALID, 7, INVALID, 6, INVALID, 5, INVALID, \ + 4, INVALID, 3, INVALID, 2, INVALID, 1, INVALID) + +#define _WRITE32_BITFIELDS_IMPL(addr, masks, values) write32(addr, values) + +#define WRITE32_BITFIELDS(addr, ...) \ + _BF_IMPL(_WRITE32_BITFIELDS_IMPL, addr, __VA_ARGS__) + +#define SET32_BITFIELDS(addr, ...) \ + _BF_IMPL(clrsetbits_le32, addr, __VA_ARGS__) + +#define EXTRACT_BITFIELD(value, name) \ + (((value) & _BF_MASK(name, 0)) >> name##_BITFIELD_SHIFT) + +#define READ32_BITFIELD(addr, name) \ + EXTRACT_BITFIELD(read32(addr), name) + #endif /* !__ROMCC__ */
#endif /* __DEVICE_MMIO_H__ */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 13:
Hmm, why is the API mixing host endianness with little-endian in the SET case? Is that intentional?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 13:
Hmm, why is the API mixing host endianness with little-endian in the SET case? Is that intentional?
I think it's just because we didn't have a clrsetbits() primitive that works directly on host endianness, and we don't have a big-endian architecture anyway. It's meant to be host endianness.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 13:
Patch Set 13:
Hmm, why is the API mixing host endianness with little-endian in the SET case? Is that intentional?
I think it's just because we didn't have a clrsetbits() primitive that works directly on host endianness, and we don't have a big-endian architecture anyway. It's meant to be host endianness.
Right. If there was clrsetbits32 I'd definitely use them. Julius and I had discussion on this and the conclusion is there's no active platform using bigendian today in coreboot (the only exception was MIPS, which was going to be dropped) so we can make the change.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 13:
Right. If there was clrsetbits32 I'd definitely use them.
Just to close the loop here, CB:36596 recently added another API for register read/modify/write (which just uses host byte order). I think it may be generally useful to have both endian-specific and host byte order functions for this (and maybe find&replace over the codebase once because I'm pretty sure all current instances of clrsetbits_le32() were just intending to use host byte order), but I don't like that they're defined in completely separate places with completely separate names right now. Maybe we should remove the update32() stuff again and instead add a clrbits32()/setbits32()/clrsetbits32() (without the le/be) API to do that instead.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 13:
Hmm, why is the API mixing host endianness with little-endian in the SET case? Is that intentional?
I think it's just because we didn't have a clrsetbits() primitive that works directly on host endianness, and we don't have a big-endian architecture anyway. It's meant to be host endianness.
Right. If there was clrsetbits32 I'd definitely use them. Julius and I had discussion on this and the conclusion is there's no active platform using bigendian today in coreboot (the only exception was MIPS, which was going to be dropped) so we can make the change.
People try to get Power9 running and even though that has configurable endianness, they have specified that the firmware runs with big-endian. I've also seen past attempts to get PowerPC support back in, for current SoCs... but everyone is running into trouble because too much code assumes little-endian by now.
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)