Re: [PATCH 13/14] drm/i915/dpio: Clean up the vlv/chv PHY register bits
On Mon, Apr 22, 2024 at 03:46:01PM +0300, Jani Nikula wrote: > On Mon, 22 Apr 2024, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Use REG_BIT() & co. for the vlv/chv DPIO PHY registers. > > > > Signed-off-by: Ville Syrjälä > > What a PITA patch to review! Aye. Sorry. > > A couple of comments inline, overall > > Reviewed-by: Jani Nikula > > [snip] > > > #define VLV_PLL_DW5(ch)_VLV_PLL((ch), 5) > > -#define DPIO_REFSEL_OVERRIDE 27 > > -#define DPIO_PLL_MODESEL_SHIFT 24 /* 3 bits */ > > -#define DPIO_BIAS_CURRENT_CTL_SHIFT 21 /* 3 bits, always 0x7 */ > > Here the shift is 21... > > > -#define DPIO_PLL_REFCLK_SEL_SHIFT16 /* 2 bits */ > > -#define DPIO_PLL_REFCLK_SEL_MASK 3 > > -#define DPIO_DRIVER_CTL_SHIFT12 /* always set to 0x8 */ > > -#define DPIO_CLK_BIAS_CTL_SHIFT 8 /* always set to 0x5 */ > > +#define DPIO_REFSEL_OVERRIDE REG_BIT(27) > > +#define DPIO_PLL_MODESEL_MASKREG_GENMASK(26, 24) > > +#define DPIO_BIAS_CURRENT_CTL_MASK REG_GENMASK(22, 20) /* always > > 0x7 */ > > ...and here it's 20. Is this is a fix to match spec or an accident? 20 looks to be the correct number. I guess I fixed it up when I originally wrote this patch, which apparently happened a few years ago. How time flies. > > Code offers no help as it's unused afaict. > > > +#define DPIO_PLL_REFCLK_SEL_MASK REG_GENMASK(17, 16) > > +#define DPIO_DRIVER_CTL_MASK REG_GENMASK(15, 12) /* always > > set to 0x8 */ > > +#define DPIO_CLK_BIAS_CTL_MASK REG_GENMASK(11, 8) /* always set to 0x5 > > */ > > > > #define VLV_PLL_DW7(ch)_VLV_PLL((ch), 7) > > > > @@ -253,101 +259,110 @@ > > #define VLV_PCS_DW0_GRP(ch)_VLV_PCS_GRP((ch), 0) > > #define VLV_PCS01_DW0(ch) _VLV_PCS((ch), 0, 0) > > #define VLV_PCS23_DW0(ch) _VLV_PCS((ch), 1, 0) > > -#define DPIO_PCS_TX_LANE2_RESET (1 << 16) > > -#define DPIO_PCS_TX_LANE1_RESET (1 << 7) > > -#define DPIO_LEFT_TXFIFO_RST_MASTER2 (1 << 4) > > -#define DPIO_RIGHT_TXFIFO_RST_MASTER2(1 << 3) > > +#define DPIO_PCS_TX_LANE2_RESET REG_BIT(16) > > +#define DPIO_PCS_TX_LANE1_RESET REG_BIT(7) > > +#define DPIO_LEFT_TXFIFO_RST_MASTER2 REG_BIT(4) > > +#define DPIO_RIGHT_TXFIFO_RST_MASTER2REG_BIT(3) > > > > -#define VLV_PCS_DW1_GRP(ch)_VLV_PCS_GRP((ch), 1) > > -#define VLV_PCS01_DW1(ch) _VLV_PCS((ch), 0, 1) > > -#define VLV_PCS23_DW1(ch) _VLV_PCS((ch), 1, 1) > > -#define CHV_PCS_REQ_SOFTRESET_EN (1 << 23) > > -#define DPIO_PCS_CLK_CRI_RXEB_EIOS_EN(1 << 22) > > -#define DPIO_PCS_CLK_CRI_RXDIGFILTSG_EN (1 << 21) > > -#define DPIO_PCS_CLK_DATAWIDTH_SHIFT (6) > > -#define DPIO_PCS_CLK_SOFT_RESET (1 << 5) > > +#define VLV_PCS_DW1_GRP(ch) _VLV_PCS_GRP((ch), 1) > > +#define VLV_PCS01_DW1(ch) _VLV_PCS((ch), 0, 1) > > +#define VLV_PCS23_DW1(ch) _VLV_PCS((ch), 1, 1) > > +#define CHV_PCS_REQ_SOFTRESET_EN REG_BIT(23) > > +#define DPIO_PCS_CLK_CRI_RXEB_EIOS_ENREG_BIT(22) > > +#define DPIO_PCS_CLK_CRI_RXDIGFILTSG_EN REG_BIT(21) > > +#define DPIO_PCS_CLK_DATAWIDTH_MASK REG_GENMASK(7, 6) > > +#define DPIO_PCS_CLK_DATAWIDTH_8_10 > > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 1) > > +#define DPIO_PCS_CLK_DATAWIDTH_16_20 > > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 2) > > +#define DPIO_PCS_CLK_DATAWIDTH_32_40 > > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 3) > > +#define DPIO_PCS_CLK_SOFT_RESET REG_BIT(5) > > > > #define VLV_PCS_DW8_GRP(ch)_VLV_PCS_GRP((ch), 8) > > #define VLV_PCS01_DW8(ch) _VLV_PCS((ch), 0, 8) > > #define VLV_PCS23_DW8(ch) _VLV_PCS((ch), 1, 8) > > -#define CHV_PCS_USEDCLKCHANNEL_OVRRIDE (1 << 20) > > -#define CHV_PCS_USEDCLKCHANNEL (1 << 21) > > +#define DPIO_PCS_USEDCLKCHANNEL REG_BIT(21) > > +#define DPIO_PCS_USEDCLKCHANNEL_OVRRIDE REG_BIT(20) > > > > -#define VLV_PCS_DW9_GRP(ch)_VLV_PCS_GRP((ch), 9) > > +#defineVLV_PCS_DW9_GRP(ch) _VLV_PCS_GRP((ch), 9) > > Is the TAB intentional here, and in a number of similar places below? No. I did spot some stray tabs before sending and thought I cleaned then all up, but somehow they keep sneaking past my radar :/ > > BR, > Jani. > > > #define VLV_PCS01_DW9(ch) _VLV_PCS((ch), 0, 9) > > #define VLV_PCS23_DW9(ch) _VLV_PCS((ch), 1, 9) > > -#define DPIO_PCS_TX2MARGIN_MASK (0x7 << 13) > > -#define DPIO_PCS_TX2MARGIN_000 (0 << 13) > > -#define DPIO_PCS_TX2MARGIN_101 (1 << 13) > > -#define DPIO_PCS_TX1MARGIN_MASK (0x7 << 10) > > -#define DPIO_PCS_TX1MARGIN_000 (0 << 10) > > -#define DPIO_PCS_TX1MARGIN_101 (1 << 10) > > +#define DPIO_PCS_TX2MARGIN_MASK REG_GENMASK(15, 13) > > +#define DPIO_PCS_TX2MARGIN_000 REG_FIELD_PRE
Re: [PATCH 13/14] drm/i915/dpio: Clean up the vlv/chv PHY register bits
On Mon, 22 Apr 2024, Ville Syrjala wrote: > From: Ville Syrjälä > > Use REG_BIT() & co. for the vlv/chv DPIO PHY registers. > > Signed-off-by: Ville Syrjälä What a PITA patch to review! A couple of comments inline, overall Reviewed-by: Jani Nikula [snip] > #define VLV_PLL_DW5(ch) _VLV_PLL((ch), 5) > -#define DPIO_REFSEL_OVERRIDE 27 > -#define DPIO_PLL_MODESEL_SHIFT 24 /* 3 bits */ > -#define DPIO_BIAS_CURRENT_CTL_SHIFT21 /* 3 bits, always 0x7 */ Here the shift is 21... > -#define DPIO_PLL_REFCLK_SEL_SHIFT 16 /* 2 bits */ > -#define DPIO_PLL_REFCLK_SEL_MASK 3 > -#define DPIO_DRIVER_CTL_SHIFT 12 /* always set to 0x8 */ > -#define DPIO_CLK_BIAS_CTL_SHIFT8 /* always set to 0x5 */ > +#define DPIO_REFSEL_OVERRIDE REG_BIT(27) > +#define DPIO_PLL_MODESEL_MASK REG_GENMASK(26, 24) > +#define DPIO_BIAS_CURRENT_CTL_MASK REG_GENMASK(22, 20) /* always 0x7 */ ...and here it's 20. Is this is a fix to match spec or an accident? Code offers no help as it's unused afaict. > +#define DPIO_PLL_REFCLK_SEL_MASK REG_GENMASK(17, 16) > +#define DPIO_DRIVER_CTL_MASK REG_GENMASK(15, 12) /* always > set to 0x8 */ > +#define DPIO_CLK_BIAS_CTL_MASK REG_GENMASK(11, 8) /* always set to 0x5 > */ > > #define VLV_PLL_DW7(ch) _VLV_PLL((ch), 7) > > @@ -253,101 +259,110 @@ > #define VLV_PCS_DW0_GRP(ch) _VLV_PCS_GRP((ch), 0) > #define VLV_PCS01_DW0(ch)_VLV_PCS((ch), 0, 0) > #define VLV_PCS23_DW0(ch)_VLV_PCS((ch), 1, 0) > -#define DPIO_PCS_TX_LANE2_RESET(1 << 16) > -#define DPIO_PCS_TX_LANE1_RESET(1 << 7) > -#define DPIO_LEFT_TXFIFO_RST_MASTER2 (1 << 4) > -#define DPIO_RIGHT_TXFIFO_RST_MASTER2 (1 << 3) > +#define DPIO_PCS_TX_LANE2_RESETREG_BIT(16) > +#define DPIO_PCS_TX_LANE1_RESETREG_BIT(7) > +#define DPIO_LEFT_TXFIFO_RST_MASTER2 REG_BIT(4) > +#define DPIO_RIGHT_TXFIFO_RST_MASTER2 REG_BIT(3) > > -#define VLV_PCS_DW1_GRP(ch) _VLV_PCS_GRP((ch), 1) > -#define VLV_PCS01_DW1(ch)_VLV_PCS((ch), 0, 1) > -#define VLV_PCS23_DW1(ch)_VLV_PCS((ch), 1, 1) > -#define CHV_PCS_REQ_SOFTRESET_EN (1 << 23) > -#define DPIO_PCS_CLK_CRI_RXEB_EIOS_EN (1 << 22) > -#define DPIO_PCS_CLK_CRI_RXDIGFILTSG_EN (1 << 21) > -#define DPIO_PCS_CLK_DATAWIDTH_SHIFT (6) > -#define DPIO_PCS_CLK_SOFT_RESET(1 << 5) > +#define VLV_PCS_DW1_GRP(ch) _VLV_PCS_GRP((ch), 1) > +#define VLV_PCS01_DW1(ch) _VLV_PCS((ch), 0, 1) > +#define VLV_PCS23_DW1(ch) _VLV_PCS((ch), 1, 1) > +#define CHV_PCS_REQ_SOFTRESET_EN REG_BIT(23) > +#define DPIO_PCS_CLK_CRI_RXEB_EIOS_EN REG_BIT(22) > +#define DPIO_PCS_CLK_CRI_RXDIGFILTSG_ENREG_BIT(21) > +#define DPIO_PCS_CLK_DATAWIDTH_MASKREG_GENMASK(7, 6) > +#define DPIO_PCS_CLK_DATAWIDTH_8_10 > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 1) > +#define DPIO_PCS_CLK_DATAWIDTH_16_20 > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 2) > +#define DPIO_PCS_CLK_DATAWIDTH_32_40 > REG_FIELD_PREP(DPIO_PCS_CLK_DATAWIDTH_MASK, 3) > +#define DPIO_PCS_CLK_SOFT_RESETREG_BIT(5) > > #define VLV_PCS_DW8_GRP(ch) _VLV_PCS_GRP((ch), 8) > #define VLV_PCS01_DW8(ch)_VLV_PCS((ch), 0, 8) > #define VLV_PCS23_DW8(ch)_VLV_PCS((ch), 1, 8) > -#define CHV_PCS_USEDCLKCHANNEL_OVRRIDE (1 << 20) > -#define CHV_PCS_USEDCLKCHANNEL (1 << 21) > +#define DPIO_PCS_USEDCLKCHANNELREG_BIT(21) > +#define DPIO_PCS_USEDCLKCHANNEL_OVRRIDEREG_BIT(20) > > -#define VLV_PCS_DW9_GRP(ch) _VLV_PCS_GRP((ch), 9) > +#define VLV_PCS_DW9_GRP(ch) _VLV_PCS_GRP((ch), 9) Is the TAB intentional here, and in a number of similar places below? BR, Jani. > #define VLV_PCS01_DW9(ch)_VLV_PCS((ch), 0, 9) > #define VLV_PCS23_DW9(ch)_VLV_PCS((ch), 1, 9) > -#define DPIO_PCS_TX2MARGIN_MASK(0x7 << 13) > -#define DPIO_PCS_TX2MARGIN_000 (0 << 13) > -#define DPIO_PCS_TX2MARGIN_101 (1 << 13) > -#define DPIO_PCS_TX1MARGIN_MASK(0x7 << 10) > -#define DPIO_PCS_TX1MARGIN_000 (0 << 10) > -#define DPIO_PCS_TX1MARGIN_101 (1 << 10) > +#define DPIO_PCS_TX2MARGIN_MASKREG_GENMASK(15, 13) > +#define DPIO_PCS_TX2MARGIN_000 REG_FIELD_PREP(DPIO_PCS_TX2MARGIN_MASK, > 0) > +#define DPIO_PCS_TX2MARGIN_101 REG_FIELD_PREP(DPIO_PCS_TX2MARGIN_MASK, > 1) > +#define DPIO_PCS_TX1MARGIN_MASKREG_GENMASK(12, 10) > +#define DPIO_PCS_TX1MARGIN_000 REG_FIELD_PREP(DPIO_PCS_TX1MARGIN_MASK, > 0) > +#define DPIO_PCS_TX1MARGIN_101 REG_FIELD_PREP(DPIO_PCS_TX1MARGIN_MASK, > 1) > > -#define VLV_PCS_DW10_GRP(ch) _VLV_PCS_GRP((ch), 10) > +#define VLV_PCS_DW10_GRP(ch)_VLV_PCS_GRP((ch), 10) > #define VLV_PCS
