| Message ID | 5bd0d22ae0a0a3d7500e63ec80073c2f476faecb.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: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@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 | 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@chris-wilson.co.uk> -Chris
On Wed, Feb 27, 2019 at 08:50:31PM +0000, Chris Wilson wrote: > Quoting Jani Nikula (2019-02-27 17:02:36) <snip> > > #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. The nice thing is that it matches the spec. The hard part is running out of fingers for wide bitfields :P
On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com> wrote: > @@ -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)))) Maybe to simplify the code we can define this macro using macro below: #define REG_BIT(__n) REG_GENMASK(__n, __n) > + > +/** > + * 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))))) > + nit: Since we are defining new set of macros, do we really have to follow naming of the underlying macros? maybe we can can have clear new names: REG_BIT(n) REG_BITS(hi,low) Thanks, Michal
On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com> > wrote: > >> @@ -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)))) > > Maybe to simplify the code we can define this macro using macro below: > > #define REG_BIT(__n) REG_GENMASK(__n, __n) I don't want to limit the macro to constant expressions (even if that's the most common use for us), and in non-constant expressions the simple shift becomes unnecessarily complicated with GENMASK. Plus there's the double evaluation of __n. > >> + >> +/** >> + * 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))))) >> + > > nit: Since we are defining new set of macros, do we really have to follow > naming of the underlying macros? maybe we can can have clear new names: > > REG_BIT(n) > REG_BITS(hi,low) We've pretty much been having this conversation ever since the first RFC was posted. It could be BITS, MASK, GENMASK, FIELD (except that clashes with REG_FIELD from regmap.h), BITFIELD, whatnot. And next thing you know, we look at REG_FIELD_PREP and REG_FIELD_GET and wonder if we should have our own names for them too. REG_BITS_PREP? REG_BITS_VALUE? REG_BITS_GET? We *might* be able to come up with internally consistent naming everyone's happy with, and have those names grow on people, but based on the discussion so far I'm not optimistic. So basically I gave up on that, and with the current proposal, the names are the same as the widely used kernel macros, with REG_ prefix added. If you know what they do, you know what these do. It's still consistent, just in a different way. Also, I pretty much expect our code outside of i915_reg.h to use a mix of our versions and the underlying ones. And I'm not sure I want to start policing people to use our versions exclusively. If the names differ only with the REG_ part, I think the mix will be much easier to live with. BR, Jani. > > Thanks, > Michal >
Quoting Jani Nikula (2019-02-28 10:07:06) > On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com> > > wrote: > > > >> @@ -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)))) > > > > Maybe to simplify the code we can define this macro using macro below: > > > > #define REG_BIT(__n) REG_GENMASK(__n, __n) > > I don't want to limit the macro to constant expressions (even if that's > the most common use for us), and in non-constant expressions the simple > shift becomes unnecessarily complicated with GENMASK. Plus there's the > double evaluation of __n. > > > > >> + > >> +/** > >> + * 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))))) > >> + > > > > nit: Since we are defining new set of macros, do we really have to follow > > naming of the underlying macros? maybe we can can have clear new names: > > > > REG_BIT(n) > > REG_BITS(hi,low) > > We've pretty much been having this conversation ever since the first RFC > was posted. It could be BITS, MASK, GENMASK, FIELD (except that clashes > with REG_FIELD from regmap.h), BITFIELD, whatnot. And next thing you > know, we look at REG_FIELD_PREP and REG_FIELD_GET and wonder if we > should have our own names for them too. REG_BITS_PREP? REG_BITS_VALUE? > REG_BITS_GET? > > We *might* be able to come up with internally consistent naming > everyone's happy with, and have those names grow on people, but based on > the discussion so far I'm not optimistic. > > So basically I gave up on that, and with the current proposal, the names > are the same as the widely used kernel macros, with REG_ prefix > added. If you know what they do, you know what these do. It's still > consistent, just in a different way. > > Also, I pretty much expect our code outside of i915_reg.h to use a mix > of our versions and the underlying ones. And I'm not sure I want to > start policing people to use our versions exclusively. If the names > differ only with the REG_ part, I think the mix will be much easier to > live with. +1 An alternative naming scheme might be: BIT_U32 GENMASK_U32 FIELD_GET_U32 FIELD_PREP_U32 (which extend naturally to 16b registers etc) and perhaps more obvious what the nature of the change over the common macros. The U is in all likelihood redundant, so even BIT32 GENMASK32 FIELD32_GET FIELD32_PREP ? But I think the central tenant is that we want to use the common naming scheme so that we can trivially go from GENMASK to REG_GENMASK/GENMASK32 and that other people reading our code already know the language (plus we want these to be part of include/linux and out of our hair!) -Chris
On Thu, 28 Feb 2019 11:12:43 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > But I think the central tenant is that we want to use the common naming > scheme so that we can trivially go from GENMASK to REG_GENMASK/GENMASK32 > and that other people reading our code already know the language (plus > we want these to be part of include/linux and out of our hair!) That's valid point. Thanks, Michal
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) #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) #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) /* * 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) @@ -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) #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) #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) #define PANEL_POWER_UP_DELAY_SHIFT 16 -#define PANEL_LIGHT_ON_DELAY_MASK 0x1fff +#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 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 #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) #define PP_REFERENCE_DIVIDER_SHIFT 8 -#define PANEL_POWER_CYCLE_DELAY_MASK 0x1f +#define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0) #define PANEL_POWER_CYCLE_DELAY_SHIFT 0 /* Panel fitting */
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@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 | 94 +++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 33 deletions(-)