| Message ID | 20241113104453.1813674-1-dnyaneshwar.bhadane@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value | expand |
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__)
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, November 13, 2024 6:34 PM > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com> > Subject: Re: [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value > > 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. I will rephrase this as per new changes. > > > > 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. #1 Sure, I am removing this does here. The reason was that earlier plan was without using sub platform macros but recently It got introduce in codebase. > > > #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. This will be removed as part of #1 comment. > > > + 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) || Condition will be change with correct macro. > > + (devid == ARL_TXX_MISC3_PLATFORM_ID) || check); } > > None of this belongs in cx0 PHY code. #3 Earlier the c20 TMDS algorithm was display version specific and generic but now we need compare with platform type for selecting correct value for tx_misc , tx_term_ctrl Please suggest correct place not to sure to place here as par of the condition below. > > + > > +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. Is intel_display_reg_defs.h is good place ?. > > > 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. Yes, tx[1] (SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1) field is 16 bit field. With tx_mics mapped to [0:7] bit , txX_dcc_cal_dac_ctrl_range [8:11] txX_dcc_bypass is 12th bit and txX_term_ctrl mapped to [13:15] bit. I will add this explanation as well in commit message. > > > > > #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? It will be removed as per comment #1. Thank you jani for reviewing. Dnyaneshwar, > > > + > > #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
On Wed, 13 Nov 2024, "Bhadane, Dnyaneshwar" <dnyaneshwar.bhadane@intel.com> wrote: >> -----Original Message----- >> From: Jani Nikula <jani.nikula@linux.intel.com> >> Sent: Wednesday, November 13, 2024 6:34 PM >> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com> >> Subject: Re: [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value >> >> 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. > I will rephrase this as per new changes. >> > >> > 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. > #1 > Sure, I am removing this does here. The reason was that earlier plan was > without using sub platform macros but recently It got introduce in codebase. >> >> > #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. > > This will be removed as part of #1 comment. >> >> > + 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) || > Condition will be change with correct macro. >> > + (devid == ARL_TXX_MISC3_PLATFORM_ID) || check); } >> >> None of this belongs in cx0 PHY code. > #3 > Earlier the c20 TMDS algorithm was display version specific and generic but now we need > compare with platform type for selecting correct value for tx_misc , tx_term_ctrl > Please suggest correct place not to sure to place here as par of the condition below. The point is, this file is about cx0 PHY implementation, it's not about platform identification. Like, what if there'll be another place that needs this? The right place *might* be intel_display_device.[ch] but it might be intel_quirks.[ch] too. Or it might be a new file under soc/. Depends. Is there really no other way than looking at host bridge? > >> > + >> > +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. > Is intel_display_reg_defs.h is good place ?. No. There's nothing like this in there. And you shouldn't make all the macros be about "TXX MISC 3". Identify the platform in one place, in a generic way, and use that information in another. Again, what if there's another part of the driver that needs the same identification? It would be really odd to call that "TXX MISC 3". >> >> > 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. > Yes, > tx[1] (SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1) field is 16 bit field. > With tx_mics mapped to [0:7] bit , txX_dcc_cal_dac_ctrl_range [8:11] > txX_dcc_bypass is 12th bit and txX_term_ctrl mapped to [13:15] bit. But we only describe the register bits and bitfields here. We don't add macros that group a lot of stuff together. You do that where the information is used. Pretty much imagine that the register macros could one day be automatically generated, and you'd only have the register fields here, nothing else. BR, Jani. > > I will add this explanation as well in commit message. >> >> > >> > #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? > It will be removed as per comment #1. > > Thank you jani for reviewing. > > Dnyaneshwar, >> >> > + >> > #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
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, November 14, 2024 2:45 PM > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: RE: [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm value > > On Wed, 13 Nov 2024, "Bhadane, Dnyaneshwar" > <dnyaneshwar.bhadane@intel.com> wrote: > >> -----Original Message----- > >> From: Jani Nikula <jani.nikula@linux.intel.com> > >> Sent: Wednesday, November 13, 2024 6:34 PM > >> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com> > >> Subject: Re: [RFC] drm/i915/cx0_phy: Update HDMI TMDS C20 algorithm > >> value > >> > >> 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. > > I will rephrase this as per new changes. > >> > > >> > 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. > > #1 > > Sure, I am removing this does here. The reason was that earlier plan > > was without using sub platform macros but recently It got introduce in > codebase. > >> > >> > #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. > > > > This will be removed as part of #1 comment. > >> > >> > + 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) || > > Condition will be change with correct macro. > >> > + (devid == ARL_TXX_MISC3_PLATFORM_ID) || check); } > >> > >> None of this belongs in cx0 PHY code. > > #3 > > Earlier the c20 TMDS algorithm was display version specific and > > generic but now we need compare with platform type for selecting > > correct value for tx_misc , tx_term_ctrl Please suggest correct place not to > sure to place here as par of the condition below. > > The point is, this file is about cx0 PHY implementation, it's not about platform > identification. Like, what if there'll be another place that needs this? > > The right place *might* be intel_display_device.[ch] but it might be > intel_quirks.[ch] too. Or it might be a new file under soc/. Depends. > > Is there really no other way than looking at host bridge? There were the ways i.e. Adding flag to VBT or Reading SoC MSR or Using other than i915 PCI id that is differentiating SoCs. After a discussion it is decided to use comparing host bridge PCI id when the SoCs having the same GMD version and drm device PCI id. > > > > >> > + > >> > +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 Does this define to be included in include/linux/pci_ids.h? is it good ?. > >> > +#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. > > Is intel_display_reg_defs.h is good place ?. > > No. There's nothing like this in there. And you shouldn't make all the macros be > about "TXX MISC 3". Identify the platform in one place, in a generic way, and > use that information in another. Yes, updating it as the generic way. > > Again, what if there's another part of the driver that needs the same > identification? It would be really odd to call that "TXX MISC 3". > > >> > >> > 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. > > Yes, > > tx[1] (SRAM_GENERIC_<A/B>_TX_CNTX_CFG_1) field is 16 bit field. > > With tx_mics mapped to [0:7] bit , txX_dcc_cal_dac_ctrl_range [8:11] > > txX_dcc_bypass is 12th bit and txX_term_ctrl mapped to [13:15] bit. > > But we only describe the register bits and bitfields here. We don't add macros > that group a lot of stuff together. You do that where the information is used. > Understood now, I will use this direct define wherever they are used. Dnyaneshwar, > Pretty much imagine that the register macros could one day be automatically > generated, and you'd only have the register fields here, nothing else. > > BR, > Jani. > > > > > I will add this explanation as well in commit message. > >> > >> > > >> > #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? > > It will be removed as per comment #1. > > > > Thank you jani for reviewing. > > > > Dnyaneshwar, > >> > >> > + > >> > #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 > > -- > Jani Nikula, Intel
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> #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) +{ + u16 devid = INTEL_DEVID(dev_priv); + 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); +} + +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)) 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)) #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 + #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__)
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(-)