Jani Nikula <jani.nik...@intel.com> writes:
> Introduce _BIT(n) to define register bits and _MASK(h, l) to define
> register field masks.
>
> We define the above as wrappers to BIT() and GENMASK() respectively to
> force u32 type to go with our register size. Additionally, the specified
> type will be helpful with follow-up to define and use register field
> values through bitfield operations.
>
> The intention is that these are easier to get right and review against
> the spec than hand rolled masks.
>
> v2: rename macros to just _BIT() and _MASK() to reduce verbosity
>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nik...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 83 ++++++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 93cbd057c07a..b6cc06b42c1a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -25,6 +25,8 @@
> #ifndef _I915_REG_H_
> #define _I915_REG_H_
>
> +#include <linux/bits.h>
> +
> /**
> * DOC: The i915 register macro definition style guide
> *
> @@ -59,15 +61,13 @@
> * significant to least significant bit. Indent the register content macros
> * using two extra spaces between ``#define`` and the macro name.
> *
> - * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Define bit
> field
> - * contents so that they are already shifted in place, and can be directly
> - * OR'd. For convenience, function-like macros may be used to define bit
> fields,
> - * but do note that the macros may be needed to read as well as write the
> - * register contents.
> + * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Use
> ``_MASK()`` to
> + * define _MASK. Define bit field contents so that they are already shifted
> in
> + * place, and can be directly OR'd. For convenience, function-like macros
> may be
> + * used to define bit fields, but do note that the macros may be needed to
> read
> + * as well as write the register contents.
> *
> - * Define bits using ``(1 << N)`` instead of ``BIT(N)``. We may change this
> in
> - * the future, but this is the prevailing style. Do **not** add ``_BIT``
> suffix
> - * to the name.
> + * Define bits using ``_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
> *
> * Group the register and its contents together without blank lines, separate
> * from other registers and their contents with one blank line.
> @@ -105,8 +105,8 @@
> * #define _FOO_A 0xf000
> * #define _FOO_B 0xf001
> * #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
> - * #define FOO_ENABLE (1 << 31)
> - * #define FOO_MODE_MASK (0xf << 16)
> + * #define FOO_ENABLE _BIT(31)
> + * #define FOO_MODE_MASK _MASK(19, 16)
> * #define FOO_MODE_SHIFT 16
> * #define FOO_MODE_BAR (0 << 16)
> * #define FOO_MODE_BAZ (1 << 16)
> @@ -116,6 +116,23 @@
> * #define GEN8_BAR _MMIO(0xb888)
> */
>
> +/*
> + * Macro for defining register bits. Bit number is 0-based.
> + *
> + * Local wrapper for BIT() to force u32. Do *not* use outside of this file.
> Use
> + * BIT() instead.
> + */
> +#define _BIT(__n) ((u32)BIT(__n))
> +
> +/*
> + * Macro for defining register field masks. Bit numbers are 0-based and the
> mask
> + * includes the high and low bits.
> + *
> + * Local wrapper for GENMASK() to force u32. Do *not* use outside of this
> + * file. Use GENMASK() instead.
> + */
> +#define _MASK(__high, __low) ((u32)GENMASK(__high, __low))
Just pondering here if BUILD_BUG_ON(__low > __high) would
pay itself off.
-Mika
> +
> typedef struct {
> u32 reg;
> } i915_reg_t;
> @@ -4641,18 +4658,18 @@ enum {
>
> #define _PP_STATUS 0x61200
> #define PP_STATUS(pps_idx) _MMIO_PPS(pps_idx, _PP_STATUS)
> -#define PP_ON (1 << 31)
> +#define PP_ON _BIT(31)
>
> #define _PP_CONTROL_1 0xc7204
> #define _PP_CONTROL_2 0xc7304
> #define ICP_PP_CONTROL(x) _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
> _PP_CONTROL_2)
> -#define POWER_CYCLE_DELAY_MASK (0x1f << 4)
> +#define POWER_CYCLE_DELAY_MASK _MASK(8, 4)
> #define POWER_CYCLE_DELAY_SHIFT 4
> -#define VDD_OVERRIDE_FORCE (1 << 3)
> -#define BACKLIGHT_ENABLE (1 << 2)
> -#define PWR_DOWN_ON_RESET (1 << 1)
> -#define PWR_STATE_TARGET (1 << 0)
> +#define VDD_OVERRIDE_FORCE _BIT(3)
> +#define BACKLIGHT_ENABLE _BIT(2)
> +#define PWR_DOWN_ON_RESET _BIT(1)
> +#define PWR_STATE_TARGET _BIT(0)
> /*
> * Indicates that all dependencies of the panel are on:
> *
> @@ -4660,14 +4677,14 @@ enum {
> * - pipe enabled
> * - LVDS/DVOB/DVOC on
> */
> -#define PP_READY (1 << 30)
> +#define PP_READY _BIT(30)
> +#define PP_SEQUENCE_MASK _MASK(29, 28)
> #define PP_SEQUENCE_NONE (0 << 28)
> #define PP_SEQUENCE_POWER_UP (1 << 28)
> #define PP_SEQUENCE_POWER_DOWN (2 << 28)
> -#define PP_SEQUENCE_MASK (3 << 28)
> #define PP_SEQUENCE_SHIFT 28
> -#define PP_CYCLE_DELAY_ACTIVE (1 << 27)
> -#define PP_SEQUENCE_STATE_MASK 0x0000000f
> +#define PP_CYCLE_DELAY_ACTIVE _BIT(27)
> +#define PP_SEQUENCE_STATE_MASK _MASK(3, 0)
> #define PP_SEQUENCE_STATE_OFF_IDLE (0x0 << 0)
> #define PP_SEQUENCE_STATE_OFF_S0_1 (0x1 << 0)
> #define PP_SEQUENCE_STATE_OFF_S0_2 (0x2 << 0)
> @@ -4680,41 +4697,41 @@ enum {
>
> #define _PP_CONTROL 0x61204
> #define PP_CONTROL(pps_idx) _MMIO_PPS(pps_idx, _PP_CONTROL)
> +#define PANEL_UNLOCK_MASK _MASK(31, 16)
> #define PANEL_UNLOCK_REGS (0xabcd << 16)
> -#define PANEL_UNLOCK_MASK (0xffff << 16)
> -#define BXT_POWER_CYCLE_DELAY_MASK 0x1f0
> +#define BXT_POWER_CYCLE_DELAY_MASK _MASK(8, 4)
> #define BXT_POWER_CYCLE_DELAY_SHIFT 4
> -#define EDP_FORCE_VDD (1 << 3)
> -#define EDP_BLC_ENABLE (1 << 2)
> -#define PANEL_POWER_RESET (1 << 1)
> -#define PANEL_POWER_ON (1 << 0)
> +#define EDP_FORCE_VDD _BIT(3)
> +#define EDP_BLC_ENABLE _BIT(2)
> +#define PANEL_POWER_RESET _BIT(1)
> +#define PANEL_POWER_ON _BIT(0)
>
> #define _PP_ON_DELAYS 0x61208
> #define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx,
> _PP_ON_DELAYS)
> #define PANEL_PORT_SELECT_SHIFT 30
> -#define PANEL_PORT_SELECT_MASK (3 << 30)
> +#define PANEL_PORT_SELECT_MASK _MASK(31, 30)
> #define PANEL_PORT_SELECT_LVDS (0 << 30)
> #define PANEL_PORT_SELECT_DPA (1 << 30)
> #define PANEL_PORT_SELECT_DPC (2 << 30)
> #define PANEL_PORT_SELECT_DPD (3 << 30)
> #define PANEL_PORT_SELECT_VLV(port) ((port) << 30)
> -#define PANEL_POWER_UP_DELAY_MASK 0x1fff0000
> +#define PANEL_POWER_UP_DELAY_MASK _MASK(28, 16)
> #define PANEL_POWER_UP_DELAY_SHIFT 16
> -#define PANEL_LIGHT_ON_DELAY_MASK 0x1fff
> +#define PANEL_LIGHT_ON_DELAY_MASK _MASK(12, 0)
> #define PANEL_LIGHT_ON_DELAY_SHIFT 0
>
> #define _PP_OFF_DELAYS 0x6120C
> #define PP_OFF_DELAYS(pps_idx) _MMIO_PPS(pps_idx,
> _PP_OFF_DELAYS)
> -#define PANEL_POWER_DOWN_DELAY_MASK 0x1fff0000
> +#define PANEL_POWER_DOWN_DELAY_MASK _MASK(28, 16)
> #define PANEL_POWER_DOWN_DELAY_SHIFT 16
> -#define PANEL_LIGHT_OFF_DELAY_MASK 0x1fff
> +#define PANEL_LIGHT_OFF_DELAY_MASK _MASK(12, 0)
> #define PANEL_LIGHT_OFF_DELAY_SHIFT 0
>
> #define _PP_DIVISOR 0x61210
> #define PP_DIVISOR(pps_idx) _MMIO_PPS(pps_idx, _PP_DIVISOR)
> -#define PP_REFERENCE_DIVIDER_MASK 0xffffff00
> +#define PP_REFERENCE_DIVIDER_MASK _MASK(31, 8)
> #define PP_REFERENCE_DIVIDER_SHIFT 8
> -#define PANEL_POWER_CYCLE_DELAY_MASK 0x1f
> +#define PANEL_POWER_CYCLE_DELAY_MASK _MASK(4, 0)
> #define PANEL_POWER_CYCLE_DELAY_SHIFT 0
>
> /* Panel fitting */
> --
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
