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)