Re: [PATCH 1/3] drm/i915: introduce REG_BIT() and REG_FIELD_MASK() to define register contents

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 03, 2018 at 07:05:21PM +0300, Jani Nikula wrote:
> Introduce REG_BIT(n) to define register bits and REG_FIELD_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, and the macro wrapper naming is
> aligned as well.
> 
> The intention is that these are easier to get right and review against
> the spec than hand rolled masks.

My first impression was that reg rage as mask would get harder to code and
review, but it is just because I'm used to the current style....
But when thinking the real case of looking to the spec and writing the code
and/or reviewing it, the range makes indeed clear.

> 
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 68 +++++++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_dp.c |  2 +-
>  2 files changed, 40 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a71c507cfb9b..ac9258769435 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
> + * ``REG_FIELD_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.

can we add a note that this is all inclusive bits on the range?

>   *
> - * 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 ``REG_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                REG_BIT(31)
> + *  #define   FOO_MODE_MASK             REG_FIELD_MASK(19, 16)
>   *  #define   FOO_MODE_SHIFT            16
>   *  #define   FOO_MODE_BAR              (0 << 16)
>   *  #define   FOO_MODE_BAZ              (1 << 16)
> @@ -116,6 +116,17 @@
>   *  #define GEN8_BAR                    _MMIO(0xb888)
>   */
>  
> +/*
> + * Macro for defining register bits. Local wrapper for BIT() to force u32.
> + */
> +#define REG_BIT(n)		((u32)BIT(n))
> +
> +/*
> + * Macro for defining register field masks. Local wrapper for GENMASK() to force
> + * u32.
> + */
> +#define REG_FIELD_MASK(h, l)	((u32)GENMASK(h, l))
> +
>  typedef struct {
>  	uint32_t reg;
>  } i915_reg_t;
> @@ -4612,7 +4623,7 @@ enum {
>  
>  #define _PP_STATUS			0x61200
>  #define PP_STATUS(pps_idx)		_MMIO_PPS(pps_idx, _PP_STATUS)
> -#define   PP_ON				(1 << 31)
> +#define   PP_ON				REG_BIT(31)
>  /*
>   * Indicates that all dependencies of the panel are on:
>   *
> @@ -4620,14 +4631,14 @@ enum {
>   * - pipe enabled
>   * - LVDS/DVOB/DVOC on
>   */
> -#define   PP_READY			(1 << 30)
> +#define   PP_READY			REG_BIT(30)
> +#define   PP_SEQUENCE_MASK		REG_FIELD_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		REG_BIT(27)
> +#define   PP_SEQUENCE_STATE_MASK	REG_FIELD_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)
> @@ -4640,42 +4651,41 @@ enum {
>  
>  #define _PP_CONTROL			0x61204
>  #define PP_CONTROL(pps_idx)		_MMIO_PPS(pps_idx, _PP_CONTROL)
> +#define  PANEL_UNLOCK_MASK		REG_FIELD_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	REG_FIELD_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_OFF		(0 << 0)
> -#define  PANEL_POWER_ON			(1 << 0)
> +#define  EDP_FORCE_VDD			REG_BIT(3)
> +#define  EDP_BLC_ENABLE			REG_BIT(2)
> +#define  PANEL_POWER_RESET		REG_BIT(1)
> +#define  PANEL_POWER_ON			REG_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		REG_FIELD_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	REG_FIELD_MASK(28, 16)
>  #define  PANEL_POWER_UP_DELAY_SHIFT	16
> -#define  PANEL_LIGHT_ON_DELAY_MASK	0x1fff
> +#define  PANEL_LIGHT_ON_DELAY_MASK	REG_FIELD_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	REG_FIELD_MASK(28, 16)
>  #define  PANEL_POWER_DOWN_DELAY_SHIFT	16
> -#define  PANEL_LIGHT_OFF_DELAY_MASK	0x1fff
> +#define  PANEL_LIGHT_OFF_DELAY_MASK	REG_FIELD_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	REG_FIELD_MASK(31, 8)
>  #define  PP_REFERENCE_DIVIDER_SHIFT	8
> -#define  PANEL_POWER_CYCLE_DELAY_MASK	0x1f
> +#define  PANEL_POWER_CYCLE_DELAY_MASK	REG_FIELD_MASK(4, 0)
>  #define  PANEL_POWER_CYCLE_DELAY_SHIFT	0
>  
>  /* Panel fitting */
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 15a981ef5966..31eef9b0e33b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1040,7 +1040,7 @@ static int edp_notify_handler(struct notifier_block *this, unsigned long code,
>  
>  		/* 0x1F write to PP_DIV_REG sets max cycle delay */
>  		I915_WRITE(pp_div_reg, pp_div | 0x1F);
> -		I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> +		I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS);
>  		msleep(intel_dp->panel_power_cycle_delay);
>  	}
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux