On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula <jani.nikula@xxxxxxxxx>
wrote:
On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote:
On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@xxxxxxxxx>
wrote:
@@ -108,9 +108,9 @@
* #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A,
_FOO_B)
* #define FOO_ENABLE REG_BIT(31)
* #define FOO_MODE_MASK REG_GENMASK(19, 16)
- * #define FOO_MODE_BAR (0 << 16)
- * #define FOO_MODE_BAZ (1 << 16)
- * #define FOO_MODE_QUX_SNB (2 << 16)
+ * #define FOO_MODE_BAR REG_FIELD_PREP(FOO_MODE_MASK,
0)
+ * #define FOO_MODE_BAZ REG_FIELD_PREP(FOO_MODE_MASK,
1)
+ * #define FOO_MODE_QUX_SNB REG_FIELD_PREP(FOO_MODE_MASK,
2)
hmm, shouldn't we define these values as:
#define FOO_MODE_BAR (0)
#define FOO_MODE_BAZ (1)
#define FOO_MODE_QUX_SNB (2)
to allow using them natively with REG_FIELD_GET/PREPARE() ?
maybe we should also consider dropping _MASK suffix?
MMIO_WRITE(...,
REG_FIELD_PREPARE(FOO_ENABLE, true) |
REG_FIELD_PREPARE(FOO_MODE, FOO_MODE_BAR))
mode = REG_FIELD_GET(FOO_MODE, MMIO_READ(...));
enabled = REG_FIELD_GET(FOO_ENABLE, MMIO_READ(...));
I would have to agree with you *if* we were writing all this from
scratch. But almost all of the existing bitfield values are defined
shifted in place, so you can OR them in place directly. I want to keep
it that way instead of creating a mix. And we have about 1k macros with
_MASK suffix too.
But since this is 'documentation' part, maybe we should push into preferred
usage model, leaving behind existing code (and let it upgrade case by case)
So, yeah, it's going to be slightly problematic to REG_FIELD_GET() a
field and compare it against a defined value for that field. I expect us
to keep using things like:
if ((val & FOO_MODE_MASK) == FOO_MODE_BAR)
Hmm, if you still want to use it this way, what's the purpose of having
pair of REG_FIELD_GET macro?
Indeed, one of the reasons for the local integer constant expression
version of REG_FIELD_PREP() is to allow it in case labels:
switch (val & FOO_MODE_MASK) {
case FOO_MODE_BAR: /* defined using REG_FIELD_PREP() */
/* ... */
}
I don't want to have to start changing these common existing conventions
throughout the driver. With the proposed approach, we can define the
registers in the new style and not change anything. We can drop _SHIFT
case by case if we move to REG_FIELD_PREP/GET usage.
But actually maybe better option would be to let old definitions and code
intact, and then later change both places at once, to follow new rules:
mode = REG_FIELD_GET(FOO_MODE_MASK, val);
switch (mode) {
case FOO_MODE_BAR: /* defined like 0 based enums */
/* ... */
}
otherwise, regardless of having new style _PREP/_GET macros, we still
be using our old convention based on _SHIFT'ed values.
As Chris replied earlier, we must take into account "that other people
reading our code already know the language" and with keeping register
values shifted, we may break style expected by _PREP/_GET.
Thanks,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
