Re: [PATCH v3 08/21] drm/i915/xe2hpd: Add new C20 PHY SRAM address

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 15 Apr 2024, Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> index bdd0c8c4ef97..23a79e911972 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -254,24 +254,67 @@
>  #define PHY_C20_VDR_CUSTOM_WIDTH	0xD02
>  #define   PHY_C20_CUSTOM_WIDTH_MASK	REG_GENMASK(1, 0)
>  #define   PHY_C20_CUSTOM_WIDTH(val)	REG_FIELD_PREP8(PHY_C20_CUSTOM_WIDTH_MASK, val)
> -#define PHY_C20_A_TX_CNTX_CFG(idx)	(0xCF2E - (idx))
> -#define PHY_C20_B_TX_CNTX_CFG(idx)	(0xCF2A - (idx))

The style absolutely everywhere is to define the values with prefix
underscore right above the macro that uses them. Please just look
around. Look at the big comment near the top of i915_reg.h.

No need for the _ADDR suffix.

Please also add a helper to choose which one to use instead of
duplicating absolutely everywhere.

Something like this:

#define _IS_XE2HPD_C20(i915)	(DISPLAY_VER_FULL(i915) == IP_VER(14, 1))

#define _MTL_C20_A_TX_CNTX_CFG		0xcf2e
#define _XE2HPD_C20_A_TX_CNTX_CFG	0xcf5e
#define PHY_C20_A_TX_CNTX_CFG(i915, idx) \
	((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_TX_CNTX_CFG : _MTL_C20_A_TX_CNTX_CFG) - (idx))

And that's enough wrapping, no need to split across four lines.

BR,
Jani.


> +
> +#define PHY_C20_A_TX_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_A_TX_CNTX_CFG_ADDR : MTL_C20_A_TX_CNTX_CFG_ADDR) - \
> +		(idx))
> +#define PHY_C20_B_TX_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_B_TX_CNTX_CFG_ADDR : MTL_C20_B_TX_CNTX_CFG_ADDR) - \
> +		(idx))
>  #define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
> -#define PHY_C20_A_CMN_CNTX_CFG(idx)	(0xCDAA - (idx))
> -#define PHY_C20_B_CMN_CNTX_CFG(idx)	(0xCDA5 - (idx))
> -#define PHY_C20_A_MPLLA_CNTX_CFG(idx)	(0xCCF0 - (idx))
> -#define PHY_C20_B_MPLLA_CNTX_CFG(idx)	(0xCCE5 - (idx))
> +#define PHY_C20_A_CMN_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_A_CMN_CNTX_CFG_ADDR : MTL_C20_A_CMN_CNTX_CFG_ADDR) - \
> +		(idx))
> +#define PHY_C20_B_CMN_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_B_CMN_CNTX_CFG_ADDR : MTL_C20_B_CMN_CNTX_CFG_ADDR) - \
> +		(idx))
> +#define PHY_C20_A_MPLLA_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_A_MPLLA_CFG_ADDR : MTL_C20_A_MPLLA_CFG_ADDR) - \
> +		(idx))
> +#define PHY_C20_B_MPLLA_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_B_MPLLA_CFG_ADDR : MTL_C20_B_MPLLA_CFG_ADDR) - \
> +		(idx))
>  #define   C20_MPLLA_FRACEN		REG_BIT(14)
>  #define   C20_FB_CLK_DIV4_EN		REG_BIT(13)
>  #define   C20_MPLLA_TX_CLK_DIV_MASK	REG_GENMASK(10, 8)
> -#define PHY_C20_A_MPLLB_CNTX_CFG(idx)	(0xCB5A - (idx))
> -#define PHY_C20_B_MPLLB_CNTX_CFG(idx)	(0xCB4E - (idx))
> +#define PHY_C20_A_MPLLB_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_A_MPLLB_CFG_ADDR : MTL_C20_A_MPLLB_CFG_ADDR) - \
> +		(idx))
> +#define PHY_C20_B_MPLLB_CNTX_CFG(i915, idx)	\
> +		(((DISPLAY_VER_FULL(i915) == IP_VER(14, 1)) ? \
> +		XE2HPD_C20_B_MPLLB_CFG_ADDR : MTL_C20_B_MPLLB_CFG_ADDR) - \
> +		(idx))
>  #define   C20_MPLLB_TX_CLK_DIV_MASK	REG_GENMASK(15, 13)
>  #define   C20_MPLLB_FRACEN		REG_BIT(13)
>  #define   C20_REF_CLK_MPLLB_DIV_MASK	REG_GENMASK(12, 10)
>  #define   C20_MULTIPLIER_MASK		REG_GENMASK(11, 0)
>  #define   C20_PHY_USE_MPLLB		REG_BIT(7)
>  
> +#define MTL_C20_A_TX_CNTX_CFG_ADDR	0xCF2E
> +#define MTL_C20_B_TX_CNTX_CFG_ADDR	0xCF2A
> +#define MTL_C20_A_CMN_CNTX_CFG_ADDR	0xCDAA
> +#define MTL_C20_B_CMN_CNTX_CFG_ADDR	0xCDA5
> +#define MTL_C20_A_MPLLA_CFG_ADDR	0xCCF0
> +#define MTL_C20_B_MPLLA_CFG_ADDR	0xCCE5
> +#define MTL_C20_A_MPLLB_CFG_ADDR	0xCB5A
> +#define MTL_C20_B_MPLLB_CFG_ADDR	0xCB4E
> +
> +#define XE2HPD_C20_A_TX_CNTX_CFG_ADDR	0xCF5E
> +#define XE2HPD_C20_B_TX_CNTX_CFG_ADDR	0xCF5A
> +#define XE2HPD_C20_A_CMN_CNTX_CFG_ADDR	0xCE8E
> +#define XE2HPD_C20_B_CMN_CNTX_CFG_ADDR	0xCE89
> +#define XE2HPD_C20_A_MPLLA_CFG_ADDR	0xCE58
> +#define XE2HPD_C20_B_MPLLA_CFG_ADDR	0xCE4D
> +#define XE2HPD_C20_A_MPLLB_CFG_ADDR	0xCCC2
> +#define XE2HPD_C20_B_MPLLB_CFG_ADDR	0xCCB6
> +
>  /* C20 Phy VSwing Masks */
>  #define C20_PHY_VSWING_PREEMPH_MASK	REG_GENMASK8(5, 0)
>  #define C20_PHY_VSWING_PREEMPH(val)	REG_FIELD_PREP8(C20_PHY_VSWING_PREEMPH_MASK, val)

-- 
Jani Nikula, Intel