| Message ID | de8eb72a267f3cc9bcc89e7b1e56109e14d28c51.1551286447.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: introduce macros to define register contents | expand |
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
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); }
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(-)