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

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

 



Quoting Jani Nikula (2019-02-27 17:02:36)
> Introduce REG_BIT(n) to define register bits and REG_GENMASK(h, l) to
> define register bitfield masks.
> 
> We define the above as wrappers to BIT() and GENMASK() respectively to
> force u32 type to go with our register size, and to add compile time
> checks on the bit numbers.
> 
> The intention is that these are easier to get right and review against
> the spec than hand rolled masks.
> 
> Convert power sequencer registers as an example.
> 
> v3:
> - rename macros to REG_BIT() and REG_GENMASK() to avoid underscore
>   prefix and to be in line with kernel macros (Chris)
> - add compile time checks (Mika)
> 
> v2:
> - rename macros to just _BIT() and _MASK() to reduce verbosity
> 
> 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 | 94 +++++++++++++++++++++------------
>  1 file changed, 61 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c9b482bc6433..e847a18067bc 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_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 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_GENMASK(19, 16)
>   *  #define   FOO_MODE_SHIFT            16
>   *  #define   FOO_MODE_BAR              (0 << 16)
>   *  #define   FOO_MODE_BAZ              (1 << 16)
> @@ -116,6 +116,34 @@
>   *  #define GEN8_BAR                    _MMIO(0xb888)
>   */
>  
> +/**
> + * REG_BIT() - Prepare a u32 bit value
> + * @__n: 0-based bit number
> + *
> + * Local wrapper for BIT() to force u32, with compile time checks.
> + *
> + * @return: Value with bit @__n set.
> + */
> +#define REG_BIT(__n)                                                   \
> +       ((u32)(BIT(__n) +                                               \
> +              BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) &&           \
> +                                ((__n) < 0 || (__n) > 31))))
> +
> +/**
> + * REG_GENMASK() - Prepare a continuous u32 bitmask
> + * @__high: 0-based high bit
> + * @__low: 0-based low bit
> + *
> + * Local wrapper for GENMASK() to force u32, with compile time checks.
> + *
> + * @return: Continuous bitmask from @__high to @__low, inclusive.
> + */
> +#define REG_GENMASK(__high, __low)                                     \
> +       ((u32)(GENMASK(__high, __low) +                                 \
> +              BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) &&        \
> +                                __builtin_constant_p(__low) &&         \
> +                                ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> +
>  typedef struct {
>         u32 reg;
>  } i915_reg_t;
> @@ -4691,18 +4719,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                                REG_BIT(31)

Ok.

>  
>  #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                REG_GENMASK(8, 4)

Ok.

>  #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            REG_BIT(3)
> +#define  BACKLIGHT_ENABLE              REG_BIT(2)
> +#define  PWR_DOWN_ON_RESET             REG_BIT(1)
> +#define  PWR_STATE_TARGET              REG_BIT(0)

Ok.

>  /*
>   * Indicates that all dependencies of the panel are on:
>   *
> @@ -4710,14 +4738,14 @@ enum {
>   * - pipe enabled
>   * - LVDS/DVOB/DVOC on
>   */
> -#define   PP_READY                     (1 << 30)
> +#define   PP_READY                     REG_BIT(30)
> +#define   PP_SEQUENCE_MASK             REG_GENMASK(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_GENMASK(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)

Ok.

> @@ -4730,41 +4758,41 @@ enum {
>  
>  #define _PP_CONTROL                    0x61204
>  #define PP_CONTROL(pps_idx)            _MMIO_PPS(pps_idx, _PP_CONTROL)
> +#define  PANEL_UNLOCK_MASK             REG_GENMASK(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_GENMASK(8, 4)

Score one for consistency.

>  #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                 REG_BIT(3)
> +#define  EDP_BLC_ENABLE                        REG_BIT(2)
> +#define  PANEL_POWER_RESET             REG_BIT(1)
> +#define  PANEL_POWER_ON                        REG_BIT(0)

Ok.

>  #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_GENMASK(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_GENMASK(28, 16)

Ok.

>  #define  PANEL_POWER_UP_DELAY_SHIFT    16
> -#define  PANEL_LIGHT_ON_DELAY_MASK     0x1fff
> +#define  PANEL_LIGHT_ON_DELAY_MASK     REG_GENMASK(12, 0)

Ok.

>  #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_GENMASK(28, 16)
>  #define  PANEL_POWER_DOWN_DELAY_SHIFT  16
> -#define  PANEL_LIGHT_OFF_DELAY_MASK    0x1fff
> +#define  PANEL_LIGHT_OFF_DELAY_MASK    REG_GENMASK(12, 0)
>  #define  PANEL_LIGHT_OFF_DELAY_SHIFT   0

Ok.

>  #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_GENMASK(31, 8)

Ok.

>  #define  PP_REFERENCE_DIVIDER_SHIFT    8
> -#define  PANEL_POWER_CYCLE_DELAY_MASK  0x1f
> +#define  PANEL_POWER_CYCLE_DELAY_MASK  REG_GENMASK(4, 0)

Ok.

I'll get used to the hi,lo convention eventually.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
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