Re: [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value

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

On Wed, 13 Nov 2024, Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com> wrote:
> In the C20 algorithm for HDMI TMDS, certain fields have been updated
> in the BSpec to set values for SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1,
> such as tx_misc and dac_ctrl_range.
> This patch covers fields that need to be set based on the platform type.
> here for xe2lpd, xe2HPD and MTL/ARL platform.
>
> Some SoCs cannot be directly distinguished by their GMD version Id,
> Specifically to set value of tx_misc, so direct device PCI IDs
> and PCI Host Bridge IDs are used for differentiation.
>
> Bspec:74165,74491
> Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 57 ++++++++++++++++---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.h  | 11 ++++
>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 19 ++++++-
>  include/drm/intel/pciids.h                    |  8 ++-
>  4 files changed, 82 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 8ad19106fee1..018add48b8ad 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -6,6 +6,7 @@
>  #include <linux/log2.h>
>  #include <linux/math64.h>
>  #include "i915_reg.h"
> +#include <drm/intel/pciids.h>

No. Do not look at PCI IDs directly inline.

>  #include "intel_cx0_phy.h"
>  #include "intel_cx0_phy_regs.h"
>  #include "intel_ddi.h"
> @@ -2164,9 +2165,55 @@ static void intel_c10pll_dump_hw_state(struct drm_i915_private *i915,
>  			    i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i + 3]);
>  }
>  
> +static bool intel_c20_tx_mics_3_platform(struct drm_i915_private *dev_priv)

No new struct drm_i915_private uses please.

> +{
> +	u16 devid = INTEL_DEVID(dev_priv);

No. Do not use INTEL_DEVID() in display code. There are no current users
and we intend to keep it that way.

> +	u16 host_bridge_pci_dev_id;
> +	struct pci_dev *pdev = NULL;
> +	bool check = false;
> +	/*
> +	 * Some SoCs have the same PCI IDs, so differentiate them based
> +	 * on the host bridge PCI device ID to use the correct txx_mics value
> +	 */
> +	while ((pdev = pci_get_class(PCI_CLASS_BRIDGE_HOST << 8, pdev)))
> +		host_bridge_pci_dev_id = pdev->device;
> +
> +	check = (pdev &&
> +		 (IS_HOST_BRIDGE_PCI_ID_TXX_MICS_3(host_bridge_pci_dev_id)));
> +
> +	return ((devid == MTL_TXX_MISC3_PLATFORM_ID) ||
> +		(devid == ARL_TXX_MISC3_PLATFORM_ID) || check);
> +}

None of this belongs in cx0 PHY code.

> +
> +static u16 intel_c20_hdmi_tmds_tx_cgf_1(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> +	u16 tx_misc;
> +	u16 tx_dcc_cal_dac_ctrl_range = 8;
> +	u16 tx_dcc_bypass = 1;
> +	u16 tx_term_ctrl;
> +
> +	if (IS_BATTLEMAGE(dev_priv)) {
> +		tx_misc = 0;
> +		tx_term_ctrl = 2;
> +
> +	} else if (DISPLAY_VER(dev_priv) >= 20) {
> +		tx_misc = 5;
> +		tx_term_ctrl = 4;
> +	} else if (IS_METEORLAKE(dev_priv)) {
> +		if (intel_c20_tx_mics_3_platform(dev_priv))
> +			tx_misc = 3;
> +		else
> +			tx_misc = 7;
> +
> +		tx_term_ctrl = 2;
> +	}
> +	return PHY_C20_A_B_TX_CNTX_CFG_1(tx_misc, tx_dcc_cal_dac_ctrl_range,
> +					 tx_dcc_bypass, tx_term_ctrl);
> +}
> +
>  static int intel_c20_compute_hdmi_tmds_pll(struct intel_crtc_state *crtc_state)
>  {
> -	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_c20pll_state *pll_state = &crtc_state->dpll_hw_state.cx0pll.c20;
>  	u64 datarate;
>  	u64 mpll_tx_clk_div;
> @@ -2176,7 +2223,6 @@ static int intel_c20_compute_hdmi_tmds_pll(struct intel_crtc_state *crtc_state)
>  	u64 mpll_multiplier;
>  	u64 mpll_fracn_quot;
>  	u64 mpll_fracn_rem;
> -	u16 tx_misc;
>  	u8  mpllb_ana_freq_vco;
>  	u8  mpll_div_multiplier;
>  
> @@ -2196,11 +2242,6 @@ static int intel_c20_compute_hdmi_tmds_pll(struct intel_crtc_state *crtc_state)
>  	mpll_div_multiplier = min_t(u8, div64_u64((vco_freq * 16 + (datarate >> 1)),
>  						  datarate), 255);
>  
> -	if (DISPLAY_VER(display) >= 20)
> -		tx_misc = 0x5;
> -	else
> -		tx_misc = 0x0;
> -
>  	if (vco_freq <= DATARATE_3000000000)
>  		mpllb_ana_freq_vco = MPLLB_ANA_FREQ_VCO_3;
>  	else if (vco_freq <= DATARATE_3500000000)
> @@ -2212,7 +2253,7 @@ static int intel_c20_compute_hdmi_tmds_pll(struct intel_crtc_state *crtc_state)
>  
>  	pll_state->clock	= crtc_state->port_clock;
>  	pll_state->tx[0]	= 0xbe88;
> -	pll_state->tx[1]	= 0x9800 | C20_PHY_TX_MISC(tx_misc);
> +	pll_state->tx[1]	= intel_c20_hdmi_tmds_tx_cgf_1(crtc_state);
>  	pll_state->tx[2]	= 0x0000;
>  	pll_state->cmn[0]	= 0x0500;
>  	pll_state->cmn[1]	= 0x0005;
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> index 9004b99bb51f..b2417c58ae20 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> @@ -9,6 +9,17 @@
>  #include <linux/types.h>
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/pci.h>
> +
> +#define HOST_BRIDGE_PCI_DEV_ID1 0x7D1C
> +#define HOST_BRIDGE_PCI_DEV_ID2 0x7D2D
> +#define HOST_BRIDGE_PCI_DEV_ID3 0x7D2E
> +#define HOST_BRIDGE_PCI_DEV_ID4 0x7D2F
> +#define IS_HOST_BRIDGE_PCI_ID_TXX_MICS_3(id)  \
> +	(((id) == HOST_BRIDGE_PCI_DEV_ID1) || \
> +	 ((id) == HOST_BRIDGE_PCI_DEV_ID2) || \
> +	 ((id) == HOST_BRIDGE_PCI_DEV_ID3) || \
> +	 ((id) == HOST_BRIDGE_PCI_DEV_ID4))
>  

None of this belongs in cx0 PHY code.

>  enum icl_port_dpll_id;
>  struct drm_i915_private;
> 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 582d6277d20c..b586e569434f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -279,9 +279,22 @@
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_TX_CNTX_CFG : _MTL_C20_A_TX_CNTX_CFG) - (idx))
>  #define PHY_C20_B_TX_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_B_TX_CNTX_CFG : _MTL_C20_B_TX_CNTX_CFG) - (idx))
> -#define   C20_PHY_TX_RATE		REG_GENMASK(2, 0)
> -#define   C20_PHY_TX_MISC_MASK		REG_GENMASK16(7, 0)
> -#define   C20_PHY_TX_MISC(val)		REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
> +#define  C20_PHY_TX_RATE REG_GENMASK(2, 0)
> +#define  C20_PHY_TX_MISC_MASK		REG_GENMASK16(7, 0)
> +#define  C20_PHY_TX_MISC(val)		REG_FIELD_PREP16(C20_PHY_TX_MISC_MASK, (val))
> +#define  C20_PHY_TX_DCC_CAL_RANGE_MASK	REG_GENMASK16(11, 8)
> +#define  C20_PHY_TX_DCC_CAL_RANGE(val) \
> +		REG_FIELD_PREP16(C20_PHY_TX_DCC_CAL_RANGE_MASK, (val))
> +#define  C20_PHY_TX_DCC_BYPASS_SET	REG_BIT(12)
> +#define  C20_PHY_TX_DCC_BYPASS(val)	(val ? C20_PHY_TX_DCC_BYPASS_SET : 0)
> +#define  C20_PHY_TX_TERM_CTL_MASK	REG_GENMASK16(15, 13)
> +#define  C20_PHY_TX_TERM_CTL(val)	REG_FIELD_PREP16(C20_PHY_TX_TERM_CTL_MASK, (val))
> +#define PHY_C20_A_B_TX_CNTX_CFG_1(tx_misc, tx_dcc_cal_dac_ctrl_range,	\
> +				  tx_dcc_bypass, tx_term_ctrl)		\
> +		(C20_PHY_TX_MISC(tx_misc) |				\
> +		 C20_PHY_TX_DCC_CAL_RANGE(tx_dcc_cal_dac_ctrl_range) |	\
> +		 C20_PHY_TX_DCC_BYPASS(tx_dcc_bypass) |			\
> +		 C20_PHY_TX_TERM_CTL(tx_term_ctrl))

Explain.

>  
>  #define PHY_C20_A_CMN_CNTX_CFG(i915, idx) \
>  		((_IS_XE2HPD_C20(i915) ? _XE2HPD_C20_A_CMN_CNTX_CFG : _MTL_C20_A_CMN_CNTX_CFG) - (idx))
> diff --git a/include/drm/intel/pciids.h b/include/drm/intel/pciids.h
> index 7632507af166..d88c58534148 100644
> --- a/include/drm/intel/pciids.h
> +++ b/include/drm/intel/pciids.h
> @@ -765,8 +765,12 @@
>  	INTEL_ATS_M75_IDS(MACRO__, ## __VA_ARGS__)
>  
>  /* ARL */
> +
> +#define ARL_TXX_MISC3_PLATFORM_ID 0x7D41
> +#define MTL_TXX_MISC3_PLATFORM_ID 0x7D45

No. Look around you. Do you see a single thing like this?

> +
>  #define INTEL_ARL_IDS(MACRO__, ...) \
> -	MACRO__(0x7D41, ## __VA_ARGS__), \
> +	MACRO__(ARL_TXX_MISC3_PLATFORM_ID, ## __VA_ARGS__), \
>  	MACRO__(0x7D51, ## __VA_ARGS__), \
>  	MACRO__(0x7D67, ## __VA_ARGS__), \
>  	MACRO__(0x7DD1, ## __VA_ARGS__), \
> @@ -775,7 +779,7 @@
>  /* MTL */
>  #define INTEL_MTL_IDS(MACRO__, ...) \
>  	MACRO__(0x7D40, ## __VA_ARGS__), \
> -	MACRO__(0x7D45, ## __VA_ARGS__), \
> +	MACRO__(MTL_TXX_MISC3_PLATFORM_ID, ## __VA_ARGS__), \
>  	MACRO__(0x7D55, ## __VA_ARGS__), \
>  	MACRO__(0x7D60, ## __VA_ARGS__), \
>  	MACRO__(0x7DD5, ## __VA_ARGS__)

-- 
Jani Nikula, Intel