[v3,2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors

Submitted by Jani Nikula on Feb. 27, 2019, 5:02 p.m.

Details

Message ID de8eb72a267f3cc9bcc89e7b1e56109e14d28c51.1551286447.git.jani.nikula@intel.com
State New
Headers show
Series "drm/i915: introduce macros to define register contents" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Feb. 27, 2019, 5:02 p.m.
bitfield.h defines FIELD_GET() and FIELD_PREP() macros to access
bitfields using the mask alone, with no need for separate shift. Indeed,
the shift is redundant.

We define REG_FIELD_GET() and REG_FIELD_PREP() wrappers for the above,
in part to force u32 and for consistency with REG_BIT() and
REG_GENMASK(), but also as we'll need to redefine REG_FIELD_PREP() in
follow-up work to make it produce integer constant expressions.

For the most part, REG_FIELD_GET() is shorter than masking followed by
shift, and arguably has more clarity.

REG_FIELD_PREP() can get more verbose than simply shifting in place, but
it does provide masking to ensure we don't overflow the mask, something
we usually don't bother with currently.

Convert power sequencer registers as an example.

v2:
- Add the REG_FIELD_GET() and REG_FIELD_PREP() wrappers to use them
  consistently from the start.

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   | 45 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp.c   | 40 +++++++++++----------------
 drivers/gpu/drm/i915/intel_lvds.c | 40 +++++++++++++--------------
 3 files changed, 64 insertions(+), 61 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e847a18067bc..1bd75770483a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -25,6 +25,7 @@ 
 #ifndef _I915_REG_H_
 #define _I915_REG_H_
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 
 /**
@@ -61,11 +62,11 @@ 
  * 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. Use
- * ``REG_GENMASK()`` 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 bit fields using ``REG_GENMASK(h, l)``. 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 ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
  *
@@ -107,7 +108,6 @@ 
  *  #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_SHIFT            16
  *  #define   FOO_MODE_BAR              (0 << 16)
  *  #define   FOO_MODE_BAZ              (1 << 16)
  *  #define   FOO_MODE_QUX_SNB          (2 << 16)
@@ -144,6 +144,30 @@ 
 				 __builtin_constant_p(__low) &&		\
 				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
 
+/**
+ * REG_FIELD_PREP() - Prepare a u32 bitfield value
+ * @__mask: shifted mask defining the field's length and position
+ * @__val: value to put in the field
+
+ * Local wrapper for FIELD_PREP() to force u32 and for consistency with
+ * REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
+ *
+ * @return: @__val masked and shifted into the field defined by @__mask.
+ */
+#define REG_FIELD_PREP(__mask, __val)	((u32)FIELD_PREP(__mask, __val))
+
+/**
+ * REG_FIELD_GET() - Extract a u32 bitfield value
+ * @__mask: shifted mask defining the field's length and position
+ * @__val: value to extract the bitfield value from
+ *
+ * Local wrapper for FIELD_GET() to force u32 and for consistency with
+ * REG_FIELD_PREP(), REG_BIT() and REG_GENMASK().
+ *
+ * @return: Masked and shifted value of the field defined by @__mask in @__val.
+ */
+#define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
+
 typedef struct {
 	u32 reg;
 } i915_reg_t;
@@ -4726,7 +4750,6 @@  enum {
 #define ICP_PP_CONTROL(x)		_MMIO(((x) == 1) ? _PP_CONTROL_1 : \
 					      _PP_CONTROL_2)
 #define  POWER_CYCLE_DELAY_MASK		REG_GENMASK(8, 4)
-#define  POWER_CYCLE_DELAY_SHIFT	4
 #define  VDD_OVERRIDE_FORCE		REG_BIT(3)
 #define  BACKLIGHT_ENABLE		REG_BIT(2)
 #define  PWR_DOWN_ON_RESET		REG_BIT(1)
@@ -4743,7 +4766,6 @@  enum {
 #define   PP_SEQUENCE_NONE		(0 << 28)
 #define   PP_SEQUENCE_POWER_UP		(1 << 28)
 #define   PP_SEQUENCE_POWER_DOWN	(2 << 28)
-#define   PP_SEQUENCE_SHIFT		28
 #define   PP_CYCLE_DELAY_ACTIVE		REG_BIT(27)
 #define   PP_SEQUENCE_STATE_MASK	REG_GENMASK(3, 0)
 #define   PP_SEQUENCE_STATE_OFF_IDLE	(0x0 << 0)
@@ -4769,7 +4791,6 @@  enum {
 
 #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		REG_GENMASK(31, 30)
 #define  PANEL_PORT_SELECT_LVDS		(0 << 30)
 #define  PANEL_PORT_SELECT_DPA		(1 << 30)
@@ -4777,23 +4798,17 @@  enum {
 #define  PANEL_PORT_SELECT_DPD		(3 << 30)
 #define  PANEL_PORT_SELECT_VLV(port)	((port) << 30)
 #define  PANEL_POWER_UP_DELAY_MASK	REG_GENMASK(28, 16)
-#define  PANEL_POWER_UP_DELAY_SHIFT	16
 #define  PANEL_LIGHT_ON_DELAY_MASK	REG_GENMASK(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	REG_GENMASK(28, 16)
-#define  PANEL_POWER_DOWN_DELAY_SHIFT	16
 #define  PANEL_LIGHT_OFF_DELAY_MASK	REG_GENMASK(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	REG_GENMASK(31, 8)
-#define  PP_REFERENCE_DIVIDER_SHIFT	8
 #define  PANEL_POWER_CYCLE_DELAY_MASK	REG_GENMASK(4, 0)
-#define  PANEL_POWER_CYCLE_DELAY_SHIFT	0
 
 /* Panel fitting */
 #define PFIT_CONTROL	_MMIO(DISPLAY_MMIO_BASE(dev_priv) + 0x61230)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1a051c0fbfe..d1d282bc4814 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6438,25 +6438,16 @@  intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
 	}
 
 	/* Pull timing values out of registers */
-	seq->t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
-		     PANEL_POWER_UP_DELAY_SHIFT;
-
-	seq->t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
-		  PANEL_LIGHT_ON_DELAY_SHIFT;
-
-	seq->t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
-		  PANEL_LIGHT_OFF_DELAY_SHIFT;
-
-	seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
-		   PANEL_POWER_DOWN_DELAY_SHIFT;
+	seq->t1_t3 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, pp_on);
+	seq->t8 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, pp_on);
+	seq->t9 = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, pp_off);
+	seq->t10 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, pp_off);
 
 	if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
 	    HAS_PCH_ICP(dev_priv)) {
-		seq->t11_t12 = ((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
-				BXT_POWER_CYCLE_DELAY_SHIFT) * 1000;
+		seq->t11_t12 = REG_FIELD_GET(BXT_POWER_CYCLE_DELAY_MASK, pp_ctl) * 1000;
 	} else {
-		seq->t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
-		       PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
+		seq->t11_t12 = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, pp_div) * 1000;
 	}
 }
 
@@ -6616,22 +6607,23 @@  intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
 		I915_WRITE(regs.pp_ctrl, pp);
 	}
 
-	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
-		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
-	pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
-		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
+	pp_on = REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, seq->t1_t3) |
+		REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, seq->t8);
+	pp_off = REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, seq->t9) |
+		REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, seq->t10);
 	/* Compute the divisor for the pp clock, simply match the Bspec
 	 * formula. */
 	if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
 	    HAS_PCH_ICP(dev_priv)) {
 		pp_div = I915_READ(regs.pp_ctrl);
 		pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
-		pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
-				<< BXT_POWER_CYCLE_DELAY_SHIFT);
+		pp_div |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK,
+					 DIV_ROUND_UP(seq->t11_t12, 1000));
 	} else {
-		pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
-		pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
-				<< PANEL_POWER_CYCLE_DELAY_SHIFT);
+		pp_div = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK,
+					(100 * div) / 2 - 1);
+		pp_div |= REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
+					 DIV_ROUND_UP(seq->t11_t12, 1000));
 	}
 
 	/* Haswell doesn't have any port selection bits for the panel
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index b4aa49768e90..646fc29e159a 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -152,24 +152,17 @@  static void intel_lvds_pps_get_hw_state(struct drm_i915_private *dev_priv,
 	pps->powerdown_on_reset = I915_READ(PP_CONTROL(0)) & PANEL_POWER_RESET;
 
 	val = I915_READ(PP_ON_DELAYS(0));
-	pps->port = (val & PANEL_PORT_SELECT_MASK) >>
-		    PANEL_PORT_SELECT_SHIFT;
-	pps->t1_t2 = (val & PANEL_POWER_UP_DELAY_MASK) >>
-		     PANEL_POWER_UP_DELAY_SHIFT;
-	pps->t5 = (val & PANEL_LIGHT_ON_DELAY_MASK) >>
-		  PANEL_LIGHT_ON_DELAY_SHIFT;
+	pps->port = REG_FIELD_GET(PANEL_PORT_SELECT_MASK, val);
+	pps->t1_t2 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, val);
+	pps->t5 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, val);
 
 	val = I915_READ(PP_OFF_DELAYS(0));
-	pps->t3 = (val & PANEL_POWER_DOWN_DELAY_MASK) >>
-		  PANEL_POWER_DOWN_DELAY_SHIFT;
-	pps->tx = (val & PANEL_LIGHT_OFF_DELAY_MASK) >>
-		  PANEL_LIGHT_OFF_DELAY_SHIFT;
+	pps->t3 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, val);
+	pps->tx = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, val);
 
 	val = I915_READ(PP_DIVISOR(0));
-	pps->divider = (val & PP_REFERENCE_DIVIDER_MASK) >>
-		       PP_REFERENCE_DIVIDER_SHIFT;
-	val = (val & PANEL_POWER_CYCLE_DELAY_MASK) >>
-	      PANEL_POWER_CYCLE_DELAY_SHIFT;
+	pps->divider = REG_FIELD_GET(PP_REFERENCE_DIVIDER_MASK, val);
+	val = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, val);
 	/*
 	 * Remove the BSpec specified +1 (100ms) offset that accounts for a
 	 * too short power-cycle delay due to the asynchronous programming of
@@ -209,15 +202,18 @@  static void intel_lvds_pps_init_hw(struct drm_i915_private *dev_priv,
 		val |= PANEL_POWER_RESET;
 	I915_WRITE(PP_CONTROL(0), val);
 
-	I915_WRITE(PP_ON_DELAYS(0), (pps->port << PANEL_PORT_SELECT_SHIFT) |
-				    (pps->t1_t2 << PANEL_POWER_UP_DELAY_SHIFT) |
-				    (pps->t5 << PANEL_LIGHT_ON_DELAY_SHIFT));
-	I915_WRITE(PP_OFF_DELAYS(0), (pps->t3 << PANEL_POWER_DOWN_DELAY_SHIFT) |
-				     (pps->tx << PANEL_LIGHT_OFF_DELAY_SHIFT));
+	val = REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
+		REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
+		REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5);
+	I915_WRITE(PP_ON_DELAYS(0), val);
 
-	val = pps->divider << PP_REFERENCE_DIVIDER_SHIFT;
-	val |= (DIV_ROUND_UP(pps->t4, 1000) + 1) <<
-	       PANEL_POWER_CYCLE_DELAY_SHIFT;
+	val = REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, pps->t3) |
+		REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, pps->tx);
+	I915_WRITE(PP_OFF_DELAYS(0), val);
+
+	val = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, pps->divider) |
+		REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
+			       DIV_ROUND_UP(pps->t4, 1000) + 1);
 	I915_WRITE(PP_DIVISOR(0), val);
 }
 

Comments

Quoting Jani Nikula (2019-02-27 17:02:37)
> bitfield.h defines FIELD_GET() and FIELD_PREP() macros to access
> bitfields using the mask alone, with no need for separate shift. Indeed,
> the shift is redundant.
> 
> We define REG_FIELD_GET() and REG_FIELD_PREP() wrappers for the above,
> in part to force u32 and for consistency with REG_BIT() and
> REG_GENMASK(), but also as we'll need to redefine REG_FIELD_PREP() in
> follow-up work to make it produce integer constant expressions.
> 
> For the most part, REG_FIELD_GET() is shorter than masking followed by
> shift, and arguably has more clarity.
> 
> REG_FIELD_PREP() can get more verbose than simply shifting in place, but
> it does provide masking to ensure we don't overflow the mask, something
> we usually don't bother with currently.
> 
> Convert power sequencer registers as an example.
> 
> v2:
> - Add the REG_FIELD_GET() and REG_FIELD_PREP() wrappers to use them
>   consistently from the start.
> 
> 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   | 45 ++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_dp.c   | 40 +++++++++++----------------
>  drivers/gpu/drm/i915/intel_lvds.c | 40 +++++++++++++--------------
>  3 files changed, 64 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e847a18067bc..1bd75770483a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -25,6 +25,7 @@
>  #ifndef _I915_REG_H_
>  #define _I915_REG_H_
>  
> +#include <linux/bitfield.h>
>  #include <linux/bits.h>
>  
>  /**
> @@ -61,11 +62,11 @@
>   * 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. Use
> - * ``REG_GENMASK()`` 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 bit fields using ``REG_GENMASK(h, l)``. 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 ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
>   *
> @@ -107,7 +108,6 @@
>   *  #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_SHIFT            16
>   *  #define   FOO_MODE_BAR              (0 << 16)
>   *  #define   FOO_MODE_BAZ              (1 << 16)
>   *  #define   FOO_MODE_QUX_SNB          (2 << 16)
> @@ -144,6 +144,30 @@
>                                  __builtin_constant_p(__low) &&         \
>                                  ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
>  
> +/**
> + * REG_FIELD_PREP() - Prepare a u32 bitfield value
> + * @__mask: shifted mask defining the field's length and position
> + * @__val: value to put in the field
> +
> + * Local wrapper for FIELD_PREP() to force u32 and for consistency with
> + * REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
> + *
> + * @return: @__val masked and shifted into the field defined by @__mask.
> + */
> +#define REG_FIELD_PREP(__mask, __val)  ((u32)FIELD_PREP(__mask, __val))
> +
> +/**
> + * REG_FIELD_GET() - Extract a u32 bitfield value
> + * @__mask: shifted mask defining the field's length and position
> + * @__val: value to extract the bitfield value from
> + *
> + * Local wrapper for FIELD_GET() to force u32 and for consistency with
> + * REG_FIELD_PREP(), REG_BIT() and REG_GENMASK().
> + *
> + * @return: Masked and shifted value of the field defined by @__mask in @__val.
> + */
> +#define REG_FIELD_GET(__mask, __val)   ((u32)FIELD_GET(__mask, __val))

I think I prefer the REG_FIELD variants already. Let's see how they work
in practice.

>  typedef struct {
>         u32 reg;
>  } i915_reg_t;
> @@ -4726,7 +4750,6 @@ enum {
>  #define ICP_PP_CONTROL(x)              _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
>                                               _PP_CONTROL_2)
>  #define  POWER_CYCLE_DELAY_MASK                REG_GENMASK(8, 4)
> -#define  POWER_CYCLE_DELAY_SHIFT       4
>  #define  VDD_OVERRIDE_FORCE            REG_BIT(3)
>  #define  BACKLIGHT_ENABLE              REG_BIT(2)
>  #define  PWR_DOWN_ON_RESET             REG_BIT(1)
> @@ -4743,7 +4766,6 @@ enum {
>  #define   PP_SEQUENCE_NONE             (0 << 28)
>  #define   PP_SEQUENCE_POWER_UP         (1 << 28)
>  #define   PP_SEQUENCE_POWER_DOWN       (2 << 28)
> -#define   PP_SEQUENCE_SHIFT            28
>  #define   PP_CYCLE_DELAY_ACTIVE                REG_BIT(27)
>  #define   PP_SEQUENCE_STATE_MASK       REG_GENMASK(3, 0)
>  #define   PP_SEQUENCE_STATE_OFF_IDLE   (0x0 << 0)
> @@ -4769,7 +4791,6 @@ enum {
>  
>  #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                REG_GENMASK(31, 30)
>  #define  PANEL_PORT_SELECT_LVDS                (0 << 30)
>  #define  PANEL_PORT_SELECT_DPA         (1 << 30)
> @@ -4777,23 +4798,17 @@ enum {
>  #define  PANEL_PORT_SELECT_DPD         (3 << 30)
>  #define  PANEL_PORT_SELECT_VLV(port)   ((port) << 30)
>  #define  PANEL_POWER_UP_DELAY_MASK     REG_GENMASK(28, 16)
> -#define  PANEL_POWER_UP_DELAY_SHIFT    16
>  #define  PANEL_LIGHT_ON_DELAY_MASK     REG_GENMASK(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   REG_GENMASK(28, 16)
> -#define  PANEL_POWER_DOWN_DELAY_SHIFT  16
>  #define  PANEL_LIGHT_OFF_DELAY_MASK    REG_GENMASK(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     REG_GENMASK(31, 8)
> -#define  PP_REFERENCE_DIVIDER_SHIFT    8
>  #define  PANEL_POWER_CYCLE_DELAY_MASK  REG_GENMASK(4, 0)
> -#define  PANEL_POWER_CYCLE_DELAY_SHIFT 0
>  
>  /* Panel fitting */
>  #define PFIT_CONTROL   _MMIO(DISPLAY_MMIO_BASE(dev_priv) + 0x61230)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..d1d282bc4814 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6438,25 +6438,16 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
>         }
>  
>         /* Pull timing values out of registers */
> -       seq->t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
> -                    PANEL_POWER_UP_DELAY_SHIFT;
> -
> -       seq->t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
> -                 PANEL_LIGHT_ON_DELAY_SHIFT;
> -
> -       seq->t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
> -                 PANEL_LIGHT_OFF_DELAY_SHIFT;
> -
> -       seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
> -                  PANEL_POWER_DOWN_DELAY_SHIFT;
> +       seq->t1_t3 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, pp_on);
> +       seq->t8 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, pp_on);
> +       seq->t9 = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, pp_off);
> +       seq->t10 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, pp_off);

That I like.

>         if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
>             HAS_PCH_ICP(dev_priv)) {
> -               seq->t11_t12 = ((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
> -                               BXT_POWER_CYCLE_DELAY_SHIFT) * 1000;
> +               seq->t11_t12 = REG_FIELD_GET(BXT_POWER_CYCLE_DELAY_MASK, pp_ctl) * 1000;
>         } else {
> -               seq->t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
> -                      PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
> +               seq->t11_t12 = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, pp_div) * 1000;
>         }

A casualty of verbosity.

>  }
>  
> @@ -6616,22 +6607,23 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
>                 I915_WRITE(regs.pp_ctrl, pp);
>         }
>  
> -       pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> -               (seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> -       pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> -                (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> +       pp_on = REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, seq->t1_t3) |
> +               REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, seq->t8);
> +       pp_off = REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, seq->t9) |
> +               REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, seq->t10);

Re-reading a few times and it is growing on me. Yes, it's on the
verbose, just wait until we say REG16_FIELD_PREP.

>         /* Compute the divisor for the pp clock, simply match the Bspec
>          * formula. */
>         if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
>             HAS_PCH_ICP(dev_priv)) {
>                 pp_div = I915_READ(regs.pp_ctrl);
>                 pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
> -               pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> -                               << BXT_POWER_CYCLE_DELAY_SHIFT);
> +               pp_div |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK,
> +                                        DIV_ROUND_UP(seq->t11_t12, 1000));
>         } else {
> -               pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
> -               pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> -                               << PANEL_POWER_CYCLE_DELAY_SHIFT);
> +               pp_div = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK,
> +                                       (100 * div) / 2 - 1);
> +               pp_div |= REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
> +                                        DIV_ROUND_UP(seq->t11_t12, 1000));
>         }
>  
>         /* Haswell doesn't have any port selection bits for the panel
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index b4aa49768e90..646fc29e159a 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -152,24 +152,17 @@ static void intel_lvds_pps_get_hw_state(struct drm_i915_private *dev_priv,
>         pps->powerdown_on_reset = I915_READ(PP_CONTROL(0)) & PANEL_POWER_RESET;
>  
>         val = I915_READ(PP_ON_DELAYS(0));
> -       pps->port = (val & PANEL_PORT_SELECT_MASK) >>
> -                   PANEL_PORT_SELECT_SHIFT;
> -       pps->t1_t2 = (val & PANEL_POWER_UP_DELAY_MASK) >>
> -                    PANEL_POWER_UP_DELAY_SHIFT;
> -       pps->t5 = (val & PANEL_LIGHT_ON_DELAY_MASK) >>
> -                 PANEL_LIGHT_ON_DELAY_SHIFT;
> +       pps->port = REG_FIELD_GET(PANEL_PORT_SELECT_MASK, val);
> +       pps->t1_t2 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, val);
> +       pps->t5 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, val);
>  
>         val = I915_READ(PP_OFF_DELAYS(0));
> -       pps->t3 = (val & PANEL_POWER_DOWN_DELAY_MASK) >>
> -                 PANEL_POWER_DOWN_DELAY_SHIFT;
> -       pps->tx = (val & PANEL_LIGHT_OFF_DELAY_MASK) >>
> -                 PANEL_LIGHT_OFF_DELAY_SHIFT;
> +       pps->t3 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, val);
> +       pps->tx = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, val);
>  
>         val = I915_READ(PP_DIVISOR(0));
> -       pps->divider = (val & PP_REFERENCE_DIVIDER_MASK) >>
> -                      PP_REFERENCE_DIVIDER_SHIFT;
> -       val = (val & PANEL_POWER_CYCLE_DELAY_MASK) >>
> -             PANEL_POWER_CYCLE_DELAY_SHIFT;
> +       pps->divider = REG_FIELD_GET(PP_REFERENCE_DIVIDER_MASK, val);
> +       val = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, val);
>         /*
>          * Remove the BSpec specified +1 (100ms) offset that accounts for a
>          * too short power-cycle delay due to the asynchronous programming of
> @@ -209,15 +202,18 @@ static void intel_lvds_pps_init_hw(struct drm_i915_private *dev_priv,
>                 val |= PANEL_POWER_RESET;
>         I915_WRITE(PP_CONTROL(0), val);
>  
> -       I915_WRITE(PP_ON_DELAYS(0), (pps->port << PANEL_PORT_SELECT_SHIFT) |
> -                                   (pps->t1_t2 << PANEL_POWER_UP_DELAY_SHIFT) |
> -                                   (pps->t5 << PANEL_LIGHT_ON_DELAY_SHIFT));
> -       I915_WRITE(PP_OFF_DELAYS(0), (pps->t3 << PANEL_POWER_DOWN_DELAY_SHIFT) |
> -                                    (pps->tx << PANEL_LIGHT_OFF_DELAY_SHIFT));
> +       val = REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
> +               REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
> +               REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5);
> +       I915_WRITE(PP_ON_DELAYS(0), val);

That wouldn't have been so bad as
          I915_WRITE(PP_ON_DELAYS(0),
	             REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
		     REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
		     REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5));
?
>  
> -       val = pps->divider << PP_REFERENCE_DIVIDER_SHIFT;
> -       val |= (DIV_ROUND_UP(pps->t4, 1000) + 1) <<
> -              PANEL_POWER_CYCLE_DELAY_SHIFT;
> +       val = REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, pps->t3) |
> +               REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, pps->tx);
> +       I915_WRITE(PP_OFF_DELAYS(0), val);
> +
> +       val = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, pps->divider) |
> +               REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
> +                              DIV_ROUND_UP(pps->t4, 1000) + 1);
>         I915_WRITE(PP_DIVISOR(0), val);
>  }

REG_FIELD_GET is enough to justify the changes, and REG_FIELD_PREP takes
a bit more getting used, but it is growing on me.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris