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.
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)
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.
BR,
Jani.
>
> Thanks,
> Michal
>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
