drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
authorJani Nikula <jani.nikula@intel.com>
Fri, 15 Mar 2019 13:56:19 +0000 (15:56 +0200)
committerJani Nikula <jani.nikula@intel.com>
Mon, 18 Mar 2019 14:27:56 +0000 (16:27 +0200)
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.

v3:
- temp variable removal (Chris)
- rebase

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/ab68f52e55e3961bde9458c0d85a12d98ef471df.1552657998.git.jani.nikula@intel.com
drivers/gpu/drm/i915/i915_reg.h
drivers/gpu/drm/i915/intel_dp.c
drivers/gpu/drm/i915/intel_lvds.c

index 4a025e3c49c43063566aff6446d0d9ef1f28464f..31a3020e369d3d6304e0306c7e23df57c016def5 100644 (file)
@@ -25,6 +25,7 @@
 #ifndef _I915_REG_H_
 #define _I915_REG_H_
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 
 /**
  * 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.
  *
  *  #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)
                                 __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;
@@ -4727,7 +4751,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)
@@ -4744,7 +4767,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)
@@ -4770,7 +4792,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)
@@ -4778,23 +4799,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)
index 47857f96c3b13461da0ab78e7510ceeb52e7cf33..35962a84b9101737b1242afb4b3b95a39f30d1a6 100644 (file)
@@ -6438,29 +6438,19 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
        pp_off = I915_READ(regs.pp_off);
 
        /* 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 (i915_mmio_reg_valid(regs.pp_div)) {
                u32 pp_div;
 
                pp_div = I915_READ(regs.pp_div);
 
-               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;
        } else {
-               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;
        }
 }
 
@@ -6620,10 +6610,10 @@ 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);
 
        /* Haswell doesn't have any port selection bits for the panel
         * power sequencer any more. */
@@ -6655,19 +6645,15 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
         * Compute the divisor for the pp clock, simply match the Bspec formula.
         */
        if (i915_mmio_reg_valid(regs.pp_div)) {
-               u32 pp_div;
-
-               pp_div = ((100 * div) / 2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
-               pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000) <<
-                          PANEL_POWER_CYCLE_DELAY_SHIFT);
-               I915_WRITE(regs.pp_div, pp_div);
+               I915_WRITE(regs.pp_div,
+                          REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, (100 * div) / 2 - 1) |
+                          REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK, DIV_ROUND_UP(seq->t11_t12, 1000)));
        } else {
                u32 pp_ctl;
 
                pp_ctl = I915_READ(regs.pp_ctrl);
                pp_ctl &= ~BXT_POWER_CYCLE_DELAY_MASK;
-               pp_ctl |= (DIV_ROUND_UP(seq->t11_t12, 1000) <<
-                          BXT_POWER_CYCLE_DELAY_SHIFT);
+               pp_ctl |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK, DIV_ROUND_UP(seq->t11_t12, 1000));
                I915_WRITE(regs.pp_ctrl, pp_ctl);
        }
 
index b4aa49768e90fb27997692d38e36824ac81327aa..6e3cf1c4ad007728f18d77db0f9662abb7443cb5 100644 (file)
@@ -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,16 +202,19 @@ 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));
+       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;
-       I915_WRITE(PP_DIVISOR(0), val);
+       I915_WRITE(PP_OFF_DELAYS(0),
+                  REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, pps->t3) |
+                  REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, pps->tx));
+
+       I915_WRITE(PP_DIVISOR(0),
+                  REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, pps->divider) |
+                  REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
+                                 DIV_ROUND_UP(pps->t4, 1000) + 1));
 }
 
 static void intel_pre_enable_lvds(struct intel_encoder *encoder,