| Message ID | 20200624193249.20725-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/display: Implement new combo phy initialization step | expand |
On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote: > This is new step that was recently added to the combo phy > initialization. > > v2: > - using intel_de_rmw() Actually, I'm not sure whether this is valid; I believe we always have to read from a lane register and can only write to the group registers: "Reads using a port group address usually cannot return correct data. For read/modify/write to a group, the read should be to one of the lane addresses, then the write to the group address." > > BSpec: 49291 > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c > index 77b04bb3ec62..115069833348 100644 > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, > if (!icl_combo_phy_enabled(dev_priv, phy)) > return false; > > + if (INTEL_GEN(dev_priv) >= 12) { > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); If I'm reading the bspec correctly, it looks like both 00 and 01 represent div2 for this register. So maybe rather than looking for 01 exactly here we should just check that bit 30 is off? > + > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), > + DCC_MODE_SELECT_MASK, > + DCC_MODE_SELECT_CONTINUOSLY); > + } > + > ret = cnl_verify_procmon_ref_values(dev_priv, phy); > > if (phy_is_master(dev_priv, phy)) { > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) > intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); > > skip_phy_misc: > + if (INTEL_GEN(dev_priv) >= 12) { We may want to keep an eye on this part of the bspec; there seems to be something a bit off with the bspec tagging of this block and I'm not 100% sure it was actually intended to apply to RKL too or not. It seems like the bspec has these steps as the very first thing, even before the "check whether the PHY is already initialized" step. But since we're checking that the bits are set the way we want in the verify function too, I don't think the ordering should matter. > + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); As noted above, maybe we should just clear bit 30 and leave bit 29 set however it already was? Matt > + > + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), > + DCC_MODE_SELECT_MASK, > + DCC_MODE_SELECT_CONTINUOSLY); > + } > + > cnl_set_procmon_ref_values(dev_priv, phy); > > if (phy_is_master(dev_priv, phy)) { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f09120cac89a..5469c9029f6d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) > #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) > #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) > +#define DCC_MODE_SELECT_MASK (0x3 << 20) > +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) > #define COMMON_KEEPER_EN (1 << 26) > #define LATENCY_OPTIM_MASK (0x3 << 2) > #define LATENCY_OPTIM_VAL(x) ((x) << 2) > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > #define N_SCALAR(x) ((x) << 24) > #define N_SCALAR_MASK (0x7F << 24) > > +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) > + > #define _ICL_DPHY_CHKN_REG 0x194 > #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) > #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7) > -- > 2.27.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote: > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote: > > This is new step that was recently added to the combo phy > > initialization. > > > > v2: > > - using intel_de_rmw() > > Actually, I'm not sure whether this is valid; I believe we always have > to read from a lane register and can only write to the group registers: > > "Reads using a port group address usually cannot return correct > data. For read/modify/write to a group, the read should be to > one of the lane addresses, then the write to the group address." Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/ > > > BSpec: 49291 > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ > > 2 files changed, 30 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > index 77b04bb3ec62..115069833348 100644 > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, > > if (!icl_combo_phy_enabled(dev_priv, phy)) > > return false; > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > If I'm reading the bspec correctly, it looks like both 00 and 01 > represent div2 for this register. So maybe rather than looking for 01 > exactly here we should just check that bit 30 is off? > > > > + > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), > > + DCC_MODE_SELECT_MASK, > > + DCC_MODE_SELECT_CONTINUOSLY); > > + } > > + > > ret = cnl_verify_procmon_ref_values(dev_priv, phy); > > > > if (phy_is_master(dev_priv, phy)) { > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) > > intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); > > > > skip_phy_misc: > > + if (INTEL_GEN(dev_priv) >= 12) { > > We may want to keep an eye on this part of the bspec; there seems to be > something a bit off with the bspec tagging of this block and I'm not > 100% sure it was actually intended to apply to RKL too or not. > > It seems like the bspec has these steps as the very first thing, even > before the "check whether the PHY is already initialized" step. > But since we're checking that the bits are set the way we want in the > verify function too, I don't think the ordering should matter. > > > + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > As noted above, maybe we should just clear bit 30 and leave bit 29 set > however it already was? > > > Matt > > > + > > + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), > > + DCC_MODE_SELECT_MASK, > > + DCC_MODE_SELECT_CONTINUOSLY); > > + } > > + > > cnl_set_procmon_ref_values(dev_priv, phy); > > > > if (phy_is_master(dev_priv, phy)) { > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index f09120cac89a..5469c9029f6d 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) > > #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) > > #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) > > +#define DCC_MODE_SELECT_MASK (0x3 << 20) > > +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) > > #define COMMON_KEEPER_EN (1 << 26) > > #define LATENCY_OPTIM_MASK (0x3 << 2) > > #define LATENCY_OPTIM_VAL(x) ((x) << 2) > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > #define N_SCALAR(x) ((x) << 24) > > #define N_SCALAR_MASK (0x7F << 24) > > > > +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) > > + > > #define _ICL_DPHY_CHKN_REG 0x194 > > #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) > > #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7) > > -- > > 2.27.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote: > On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote: > > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote: > > > This is new step that was recently added to the combo phy > > > initialization. > > > > > > v2: > > > - using intel_de_rmw() > > > > Actually, I'm not sure whether this is valid; I believe we always have > > to read from a lane register and can only write to the group registers: > > > > "Reads using a port group address usually cannot return correct > > data. For read/modify/write to a group, the read should be to > > one of the lane addresses, then the write to the group address." > > Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/ Ah, I overlooked that, thanks. I think my other comments below may still apply to v3 though. Matt > > > > > > BSpec: 49291 > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ > > > 2 files changed, 30 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > index 77b04bb3ec62..115069833348 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, > > > if (!icl_combo_phy_enabled(dev_priv, phy)) > > > return false; > > > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > If I'm reading the bspec correctly, it looks like both 00 and 01 > > represent div2 for this register. So maybe rather than looking for 01 > > exactly here we should just check that bit 30 is off? > > > > > > > + > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), > > > + DCC_MODE_SELECT_MASK, > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > + } > > > + > > > ret = cnl_verify_procmon_ref_values(dev_priv, phy); > > > > > > if (phy_is_master(dev_priv, phy)) { > > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) > > > intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); > > > > > > skip_phy_misc: > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > > We may want to keep an eye on this part of the bspec; there seems to be > > something a bit off with the bspec tagging of this block and I'm not > > 100% sure it was actually intended to apply to RKL too or not. > > > > It seems like the bspec has these steps as the very first thing, even > > before the "check whether the PHY is already initialized" step. > > But since we're checking that the bits are set the way we want in the > > verify function too, I don't think the ordering should matter. > > > > > + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > As noted above, maybe we should just clear bit 30 and leave bit 29 set > > however it already was? > > > > > > Matt > > > > > + > > > + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), > > > + DCC_MODE_SELECT_MASK, > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > + } > > > + > > > cnl_set_procmon_ref_values(dev_priv, phy); > > > > > > if (phy_is_master(dev_priv, phy)) { > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index f09120cac89a..5469c9029f6d 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) > > > #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) > > > #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) > > > +#define DCC_MODE_SELECT_MASK (0x3 << 20) > > > +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) > > > #define COMMON_KEEPER_EN (1 << 26) > > > #define LATENCY_OPTIM_MASK (0x3 << 2) > > > #define LATENCY_OPTIM_VAL(x) ((x) << 2) > > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > #define N_SCALAR(x) ((x) << 24) > > > #define N_SCALAR_MASK (0x7F << 24) > > > > > > +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) > > > + > > > #define _ICL_DPHY_CHKN_REG 0x194 > > > #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) > > > #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7) > > > -- > > > 2.27.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2020-07-06 at 16:28 -0700, Matt Roper wrote: > On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote: > > On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote: > > > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote: > > > > This is new step that was recently added to the combo phy > > > > initialization. > > > > > > > > v2: > > > > - using intel_de_rmw() > > > > > > Actually, I'm not sure whether this is valid; I believe we always have > > > to read from a lane register and can only write to the group registers: > > > > > > "Reads using a port group address usually cannot return correct > > > data. For read/modify/write to a group, the read should be to > > > one of the lane addresses, then the write to the group address." > > > > Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/ > > Ah, I overlooked that, thanks. I think my other comments below may > still apply to v3 though. Ops missed those, answers bellow. > > > Matt > > > > > BSpec: 49291 > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ > > > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ > > > > 2 files changed, 30 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > index 77b04bb3ec62..115069833348 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, > > > > if (!icl_combo_phy_enabled(dev_priv, phy)) > > > > return false; > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > > > If I'm reading the bspec correctly, it looks like both 00 and 01 > > > represent div2 for this register. So maybe rather than looking for 01 > > > exactly here we should just check that bit 30 is off? That is a micro optimization that would cause loss in code readability, someone casually reading might get confused of why only bit 30 is checked, also we would need to add a different macro? Don't look worthy. The default value of bit 31 is off so it will trigger the phy reprogram anyways. > > > > > > > > > > + > > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), > > > > + DCC_MODE_SELECT_MASK, > > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > > + } > > > > + > > > > ret = cnl_verify_procmon_ref_values(dev_priv, phy); > > > > > > > > if (phy_is_master(dev_priv, phy)) { > > > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) > > > > intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); > > > > > > > > skip_phy_misc: > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > > > > We may want to keep an eye on this part of the bspec; there seems to be > > > something a bit off with the bspec tagging of this block and I'm not > > > 100% sure it was actually intended to apply to RKL too or not. Ok, I'm receiving notifications of this page, will update if needed but it would be odd only RKL not needing this sequence while TGL and DG1 need it. > > > > > > It seems like the bspec has these steps as the very first thing, even > > > before the "check whether the PHY is already initialized" step. > > > But since we're checking that the bits are set the way we want in the > > > verify function too, I don't think the ordering should matter. Yep, re program it each time is a waste of time. > > > > > > > + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > > > As noted above, maybe we should just clear bit 30 and leave bit 29 set > > > however it already was? > > > > > > > > > Matt > > > > > > > + > > > > + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), > > > > + DCC_MODE_SELECT_MASK, > > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > > + } > > > > + > > > > cnl_set_procmon_ref_values(dev_priv, phy); > > > > > > > > if (phy_is_master(dev_priv, phy)) { > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > index f09120cac89a..5469c9029f6d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > > #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) > > > > #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) > > > > #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) > > > > +#define DCC_MODE_SELECT_MASK (0x3 << 20) > > > > +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) > > > > #define COMMON_KEEPER_EN (1 << 26) > > > > #define LATENCY_OPTIM_MASK (0x3 << 2) > > > > #define LATENCY_OPTIM_VAL(x) ((x) << 2) > > > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > > #define N_SCALAR(x) ((x) << 24) > > > > #define N_SCALAR_MASK (0x7F << 24) > > > > > > > > +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) > > > > + > > > > #define _ICL_DPHY_CHKN_REG 0x194 > > > > #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) > > > > #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7) > > > > -- > > > > 2.27.0 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 06, 2020 at 05:14:04PM -0700, Souza, Jose wrote: > On Mon, 2020-07-06 at 16:28 -0700, Matt Roper wrote: > > On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote: > > > On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote: > > > > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote: > > > > > This is new step that was recently added to the combo phy > > > > > initialization. > > > > > > > > > > v2: > > > > > - using intel_de_rmw() > > > > > > > > Actually, I'm not sure whether this is valid; I believe we always have > > > > to read from a lane register and can only write to the group registers: > > > > > > > > "Reads using a port group address usually cannot return correct > > > > data. For read/modify/write to a group, the read should be to > > > > one of the lane addresses, then the write to the group address." > > > > > > Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/ > > > > Ah, I overlooked that, thanks. I think my other comments below may > > still apply to v3 though. > > Ops missed those, answers bellow. > > > > > > > Matt > > > > > > > BSpec: 49291 > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ > > > > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ > > > > > 2 files changed, 30 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > > index 77b04bb3ec62..115069833348 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c > > > > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, > > > > > if (!icl_combo_phy_enabled(dev_priv, phy)) > > > > > return false; > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > > > > > If I'm reading the bspec correctly, it looks like both 00 and 01 > > > > represent div2 for this register. So maybe rather than looking for 01 > > > > exactly here we should just check that bit 30 is off? > > That is a micro optimization that would cause loss in code readability, someone casually reading might get confused of why only bit 30 is checked, > also we would need to add a different macro? Don't look worthy. > The default value of bit 31 is off so it will trigger the phy reprogram anyways. My main thinking was that the BIOS might have already programmed this register according to the new bspec directions, but chosen 00 instead of 01 as its value. So testing a "CLK_DIV_SEL_HIGHDIV" rather than CLK_SEL_MASK would allow us to avoid an unnecessary reprogram. But as you said, it probably doesn't hurt anything to reprogram more often than we need to, so we can probably leave it as-is. Might be worth adding some extra explanation about that to the commit message. But otherwise, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > > > > > > > > > + > > > > > + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), > > > > > + DCC_MODE_SELECT_MASK, > > > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > > > + } > > > > > + > > > > > ret = cnl_verify_procmon_ref_values(dev_priv, phy); > > > > > > > > > > if (phy_is_master(dev_priv, phy)) { > > > > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) > > > > > intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); > > > > > > > > > > skip_phy_misc: > > > > > + if (INTEL_GEN(dev_priv) >= 12) { > > > > > > > > We may want to keep an eye on this part of the bspec; there seems to be > > > > something a bit off with the bspec tagging of this block and I'm not > > > > 100% sure it was actually intended to apply to RKL too or not. > > Ok, I'm receiving notifications of this page, will update if needed but it would be odd only RKL not needing this sequence while TGL and DG1 need it. > > > > > > > > > It seems like the bspec has these steps as the very first thing, even > > > > before the "check whether the PHY is already initialized" step. > > > > But since we're checking that the bits are set the way we want in the > > > > verify function too, I don't think the ordering should matter. > > Yep, re program it each time is a waste of time. > > > > > > > > > > + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_SEL | > > > > > + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); > > > > > > > > As noted above, maybe we should just clear bit 30 and leave bit 29 set > > > > however it already was? > > > > > > > > > > > > Matt > > > > > > > > > + > > > > > + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), > > > > > + DCC_MODE_SELECT_MASK, > > > > > + DCC_MODE_SELECT_CONTINUOSLY); > > > > > + } > > > > > + > > > > > cnl_set_procmon_ref_values(dev_priv, phy); > > > > > > > > > > if (phy_is_master(dev_priv, phy)) { > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > > index f09120cac89a..5469c9029f6d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > > > #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) > > > > > #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) > > > > > #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) > > > > > +#define DCC_MODE_SELECT_MASK (0x3 << 20) > > > > > +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) > > > > > #define COMMON_KEEPER_EN (1 << 26) > > > > > #define LATENCY_OPTIM_MASK (0x3 << 2) > > > > > #define LATENCY_OPTIM_VAL(x) ((x) << 2) > > > > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > > > > #define N_SCALAR(x) ((x) << 24) > > > > > #define N_SCALAR_MASK (0x7F << 24) > > > > > > > > > > +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) > > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) > > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) > > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) > > > > > + > > > > > #define _ICL_DPHY_CHKN_REG 0x194 > > > > > #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) > > > > > #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7) > > > > > -- > > > > > 2.27.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c index 77b04bb3ec62..115069833348 100644 --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, if (!icl_combo_phy_enabled(dev_priv, phy)) return false; + if (INTEL_GEN(dev_priv) >= 12) { + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy), + ICL_PORT_TX_DW8_ODCC_CLK_SEL | + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, + ICL_PORT_TX_DW8_ODCC_CLK_SEL | + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); + + ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy), + DCC_MODE_SELECT_MASK, + DCC_MODE_SELECT_CONTINUOSLY); + } + ret = cnl_verify_procmon_ref_values(dev_priv, phy); if (phy_is_master(dev_priv, phy)) { @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv) intel_de_write(dev_priv, ICL_PHY_MISC(phy), val); skip_phy_misc: + if (INTEL_GEN(dev_priv) >= 12) { + intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy), + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, + ICL_PORT_TX_DW8_ODCC_CLK_SEL | + ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2); + + intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy), + DCC_MODE_SELECT_MASK, + DCC_MODE_SELECT_CONTINUOSLY); + } + cnl_set_procmon_ref_values(dev_priv, phy); if (phy_is_master(dev_priv, phy)) { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f09120cac89a..5469c9029f6d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define ICL_PORT_PCS_DW1_AUX(phy) _MMIO(_ICL_PORT_PCS_DW_AUX(1, phy)) #define ICL_PORT_PCS_DW1_GRP(phy) _MMIO(_ICL_PORT_PCS_DW_GRP(1, phy)) #define ICL_PORT_PCS_DW1_LN0(phy) _MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy)) +#define DCC_MODE_SELECT_MASK (0x3 << 20) +#define DCC_MODE_SELECT_CONTINUOSLY (0x3 << 20) #define COMMON_KEEPER_EN (1 << 26) #define LATENCY_OPTIM_MASK (0x3 << 2) #define LATENCY_OPTIM_VAL(x) ((x) << 2) @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define N_SCALAR(x) ((x) << 24) #define N_SCALAR_MASK (0x7F << 24) +#define ICL_PORT_TX_DW8_GRP(phy) _MMIO(_ICL_PORT_TX_DW_GRP(8, phy)) +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL REG_BIT(31) +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK REG_GENMASK(30, 29) +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2 REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1) + #define _ICL_DPHY_CHKN_REG 0x194 #define ICL_DPHY_CHKN(port) _MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG) #define ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP REG_BIT(7)
This is new step that was recently added to the combo phy initialization. v2: - using intel_de_rmw() BSpec: 49291 Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- .../gpu/drm/i915/display/intel_combo_phy.c | 23 +++++++++++++++++++ drivers/gpu/drm/i915/i915_reg.h | 7 ++++++ 2 files changed, 30 insertions(+)