Re: [PATCH v2 2/4] drm/i915: introduce _BIT() and _MASK() to define register contents

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Jani Nikula <jani.nikula@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 <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@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