| Message ID | 20191202213646.258752-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CI] drm/i915/display/tgl: Do not program clockgating | expand |
On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> wrote: > Talked with HW team and this is a left over, driver should not > program clockgating, dekel firmware will be reponsible for any > clockgating programing. > > v2: > Added WARN_ON > > BSpec issue: 20885 > BSpec: 49292 > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 54 +++++++----------------- > 1 file changed, 15 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index a976606d21c7..66052a9f1474 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -3167,6 +3167,10 @@ icl_phy_set_clock_gating(struct intel_digital_port *dig_port, bool enable) > u32 val, bits; > int ln; > > + /* See "PHY Clockgating programming" note */ Where? > + if (WARN_ON(INTEL_GEN(dev_priv) >= 12)) > + return; > + > if (tc_port == PORT_TC_NONE) > return; > > @@ -3175,39 +3179,26 @@ icl_phy_set_clock_gating(struct intel_digital_port *dig_port, bool enable) > MG_DP_MODE_CFG_GAONPWR_GATING; > > for (ln = 0; ln < 2; ln++) { > - if (INTEL_GEN(dev_priv) >= 12) { > - I915_WRITE(HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port, ln)); > - val = I915_READ(DKL_DP_MODE(tc_port)); > - } else { > - val = I915_READ(MG_DP_MODE(ln, tc_port)); > - } > + val = I915_READ(MG_DP_MODE(ln, tc_port)); > > if (enable) > val |= bits; > else > val &= ~bits; > > - if (INTEL_GEN(dev_priv) >= 12) > - I915_WRITE(DKL_DP_MODE(tc_port), val); > - else > - I915_WRITE(MG_DP_MODE(ln, tc_port), val); > + I915_WRITE(MG_DP_MODE(ln, tc_port), val); > } > > - if (INTEL_GEN(dev_priv) == 11) { > - bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | > - MG_MISC_SUS0_CFG_CL2PWR_GATING | > - MG_MISC_SUS0_CFG_GAONPWR_GATING | > - MG_MISC_SUS0_CFG_TRPWR_GATING | > - MG_MISC_SUS0_CFG_CL1PWR_GATING | > - MG_MISC_SUS0_CFG_DGPWR_GATING; > + bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | MG_MISC_SUS0_CFG_CL2PWR_GATING | > + MG_MISC_SUS0_CFG_GAONPWR_GATING | MG_MISC_SUS0_CFG_TRPWR_GATING | > + MG_MISC_SUS0_CFG_CL1PWR_GATING | MG_MISC_SUS0_CFG_DGPWR_GATING; > > - val = I915_READ(MG_MISC_SUS0(tc_port)); > - if (enable) > - val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); > - else > - val &= ~(bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); > - I915_WRITE(MG_MISC_SUS0(tc_port), val); > - } > + val = I915_READ(MG_MISC_SUS0(tc_port)); > + if (enable) > + val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); > + else > + val &= ~(bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); > + I915_WRITE(MG_MISC_SUS0(tc_port), val); > } > > static void > @@ -3508,12 +3499,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder, > * down this function. > */ > > - /* > - * 7.d Type C with DP alternate or fixed/legacy/static connection - > - * Disable PHY clock gating per Type-C DDI Buffer page > - */ > - icl_phy_set_clock_gating(dig_port, false); > - > /* 7.e Configure voltage swing and related IO settings */ > tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level, > encoder->type); > @@ -3565,15 +3550,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder, > if (!is_trans_port_sync_mode(crtc_state)) > intel_dp_stop_link_train(intel_dp); > > - /* > - * TODO: enable clock gating > - * > - * It is not written in DP enabling sequence but "PHY Clockgating > - * programming" states that clock gating should be enabled after the > - * link training but doing so causes all the following trainings to fail > - * so not enabling it for now. > - */ > - > /* 7.l Configure and enable FEC if needed */ > intel_ddi_enable_fec(encoder, crtc_state); > intel_dsc_enable(encoder, crtc_state);
On Tue, 2019-12-03 at 10:20 +0200, Jani Nikula wrote: > On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> > wrote: > > Talked with HW team and this is a left over, driver should not > > program clockgating, dekel firmware will be reponsible for any > > clockgating programing. > > > > v2: > > Added WARN_ON > > > > BSpec issue: 20885 > > BSpec: 49292 > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 54 +++++++------------- > > ---- > > 1 file changed, 15 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index a976606d21c7..66052a9f1474 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -3167,6 +3167,10 @@ icl_phy_set_clock_gating(struct > > intel_digital_port *dig_port, bool enable) > > u32 val, bits; > > int ln; > > > > + /* See "PHY Clockgating programming" note */ > > Where? BSpec: 49292, PHY Clockgating programming: Display driver should not program the following bits. > > > + if (WARN_ON(INTEL_GEN(dev_priv) >= 12)) > > + return; > > + > > if (tc_port == PORT_TC_NONE) > > return; > > > > @@ -3175,39 +3179,26 @@ icl_phy_set_clock_gating(struct > > intel_digital_port *dig_port, bool enable) > > MG_DP_MODE_CFG_GAONPWR_GATING; > > > > for (ln = 0; ln < 2; ln++) { > > - if (INTEL_GEN(dev_priv) >= 12) { > > - I915_WRITE(HIP_INDEX_REG(tc_port), > > HIP_INDEX_VAL(tc_port, ln)); > > - val = I915_READ(DKL_DP_MODE(tc_port)); > > - } else { > > - val = I915_READ(MG_DP_MODE(ln, tc_port)); > > - } > > + val = I915_READ(MG_DP_MODE(ln, tc_port)); > > > > if (enable) > > val |= bits; > > else > > val &= ~bits; > > > > - if (INTEL_GEN(dev_priv) >= 12) > > - I915_WRITE(DKL_DP_MODE(tc_port), val); > > - else > > - I915_WRITE(MG_DP_MODE(ln, tc_port), val); > > + I915_WRITE(MG_DP_MODE(ln, tc_port), val); > > } > > > > - if (INTEL_GEN(dev_priv) == 11) { > > - bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | > > - MG_MISC_SUS0_CFG_CL2PWR_GATING | > > - MG_MISC_SUS0_CFG_GAONPWR_GATING | > > - MG_MISC_SUS0_CFG_TRPWR_GATING | > > - MG_MISC_SUS0_CFG_CL1PWR_GATING | > > - MG_MISC_SUS0_CFG_DGPWR_GATING; > > + bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | > > MG_MISC_SUS0_CFG_CL2PWR_GATING | > > + MG_MISC_SUS0_CFG_GAONPWR_GATING | > > MG_MISC_SUS0_CFG_TRPWR_GATING | > > + MG_MISC_SUS0_CFG_CL1PWR_GATING | > > MG_MISC_SUS0_CFG_DGPWR_GATING; > > > > - val = I915_READ(MG_MISC_SUS0(tc_port)); > > - if (enable) > > - val |= (bits | > > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); > > - else > > - val &= ~(bits | > > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); > > - I915_WRITE(MG_MISC_SUS0(tc_port), val); > > - } > > + val = I915_READ(MG_MISC_SUS0(tc_port)); > > + if (enable) > > + val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); > > + else > > + val &= ~(bits | > > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); > > + I915_WRITE(MG_MISC_SUS0(tc_port), val); > > } > > > > static void > > @@ -3508,12 +3499,6 @@ static void tgl_ddi_pre_enable_dp(struct > > intel_encoder *encoder, > > * down this function. > > */ > > > > - /* > > - * 7.d Type C with DP alternate or fixed/legacy/static > > connection - > > - * Disable PHY clock gating per Type-C DDI Buffer page > > - */ > > - icl_phy_set_clock_gating(dig_port, false); > > - > > /* 7.e Configure voltage swing and related IO settings */ > > tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level, > > encoder->type); > > @@ -3565,15 +3550,6 @@ static void tgl_ddi_pre_enable_dp(struct > > intel_encoder *encoder, > > if (!is_trans_port_sync_mode(crtc_state)) > > intel_dp_stop_link_train(intel_dp); > > > > - /* > > - * TODO: enable clock gating > > - * > > - * It is not written in DP enabling sequence but "PHY > > Clockgating > > - * programming" states that clock gating should be enabled > > after the > > - * link training but doing so causes all the following > > trainings to fail > > - * so not enabling it for now. > > - */ > > - > > /* 7.l Configure and enable FEC if needed */ > > intel_ddi_enable_fec(encoder, crtc_state); > > intel_dsc_enable(encoder, crtc_state);
On Tue, 03 Dec 2019, "Souza, Jose" <jose.souza@intel.com> wrote: > On Tue, 2019-12-03 at 10:20 +0200, Jani Nikula wrote: >> On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> >> wrote: >> > Talked with HW team and this is a left over, driver should not >> > program clockgating, dekel firmware will be reponsible for any >> > clockgating programing. >> > >> > v2: >> > Added WARN_ON >> > >> > BSpec issue: 20885 >> > BSpec: 49292 >> > >> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> > Cc: Matt Roper <matthew.d.roper@intel.com> >> > Cc: Jani Nikula <jani.nikula@intel.com> >> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_ddi.c | 54 +++++++------------- >> > ---- >> > 1 file changed, 15 insertions(+), 39 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c >> > b/drivers/gpu/drm/i915/display/intel_ddi.c >> > index a976606d21c7..66052a9f1474 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> > @@ -3167,6 +3167,10 @@ icl_phy_set_clock_gating(struct >> > intel_digital_port *dig_port, bool enable) >> > u32 val, bits; >> > int ln; >> > >> > + /* See "PHY Clockgating programming" note */ >> >> Where? > > BSpec: 49292, PHY Clockgating programming: Display driver should not > program the following bits. In a code comment, people would look for the note in the source. That's certainly what *I* did. And it's pretty rude to have a reference to internal documentation in a public source tree. If you think we need to have the comment, please paraphrase the spec. For example, just "Per Bspec we shouldn't do this on gen12+" or something is better than an obscure reference. BR, Jani. > >> >> > + if (WARN_ON(INTEL_GEN(dev_priv) >= 12)) >> > + return; >> > + >> > if (tc_port == PORT_TC_NONE) >> > return; >> > >> > @@ -3175,39 +3179,26 @@ icl_phy_set_clock_gating(struct >> > intel_digital_port *dig_port, bool enable) >> > MG_DP_MODE_CFG_GAONPWR_GATING; >> > >> > for (ln = 0; ln < 2; ln++) { >> > - if (INTEL_GEN(dev_priv) >= 12) { >> > - I915_WRITE(HIP_INDEX_REG(tc_port), >> > HIP_INDEX_VAL(tc_port, ln)); >> > - val = I915_READ(DKL_DP_MODE(tc_port)); >> > - } else { >> > - val = I915_READ(MG_DP_MODE(ln, tc_port)); >> > - } >> > + val = I915_READ(MG_DP_MODE(ln, tc_port)); >> > >> > if (enable) >> > val |= bits; >> > else >> > val &= ~bits; >> > >> > - if (INTEL_GEN(dev_priv) >= 12) >> > - I915_WRITE(DKL_DP_MODE(tc_port), val); >> > - else >> > - I915_WRITE(MG_DP_MODE(ln, tc_port), val); >> > + I915_WRITE(MG_DP_MODE(ln, tc_port), val); >> > } >> > >> > - if (INTEL_GEN(dev_priv) == 11) { >> > - bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | >> > - MG_MISC_SUS0_CFG_CL2PWR_GATING | >> > - MG_MISC_SUS0_CFG_GAONPWR_GATING | >> > - MG_MISC_SUS0_CFG_TRPWR_GATING | >> > - MG_MISC_SUS0_CFG_CL1PWR_GATING | >> > - MG_MISC_SUS0_CFG_DGPWR_GATING; >> > + bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | >> > MG_MISC_SUS0_CFG_CL2PWR_GATING | >> > + MG_MISC_SUS0_CFG_GAONPWR_GATING | >> > MG_MISC_SUS0_CFG_TRPWR_GATING | >> > + MG_MISC_SUS0_CFG_CL1PWR_GATING | >> > MG_MISC_SUS0_CFG_DGPWR_GATING; >> > >> > - val = I915_READ(MG_MISC_SUS0(tc_port)); >> > - if (enable) >> > - val |= (bits | >> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); >> > - else >> > - val &= ~(bits | >> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); >> > - I915_WRITE(MG_MISC_SUS0(tc_port), val); >> > - } >> > + val = I915_READ(MG_MISC_SUS0(tc_port)); >> > + if (enable) >> > + val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); >> > + else >> > + val &= ~(bits | >> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); >> > + I915_WRITE(MG_MISC_SUS0(tc_port), val); >> > } >> > >> > static void >> > @@ -3508,12 +3499,6 @@ static void tgl_ddi_pre_enable_dp(struct >> > intel_encoder *encoder, >> > * down this function. >> > */ >> > >> > - /* >> > - * 7.d Type C with DP alternate or fixed/legacy/static >> > connection - >> > - * Disable PHY clock gating per Type-C DDI Buffer page >> > - */ >> > - icl_phy_set_clock_gating(dig_port, false); >> > - >> > /* 7.e Configure voltage swing and related IO settings */ >> > tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level, >> > encoder->type); >> > @@ -3565,15 +3550,6 @@ static void tgl_ddi_pre_enable_dp(struct >> > intel_encoder *encoder, >> > if (!is_trans_port_sync_mode(crtc_state)) >> > intel_dp_stop_link_train(intel_dp); >> > >> > - /* >> > - * TODO: enable clock gating >> > - * >> > - * It is not written in DP enabling sequence but "PHY >> > Clockgating >> > - * programming" states that clock gating should be enabled >> > after the >> > - * link training but doing so causes all the following >> > trainings to fail >> > - * so not enabling it for now. >> > - */ >> > - >> > /* 7.l Configure and enable FEC if needed */ >> > intel_ddi_enable_fec(encoder, crtc_state); >> > intel_dsc_enable(encoder, crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index a976606d21c7..66052a9f1474 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -3167,6 +3167,10 @@ icl_phy_set_clock_gating(struct intel_digital_port *dig_port, bool enable) u32 val, bits; int ln; + /* See "PHY Clockgating programming" note */ + if (WARN_ON(INTEL_GEN(dev_priv) >= 12)) + return; + if (tc_port == PORT_TC_NONE) return; @@ -3175,39 +3179,26 @@ icl_phy_set_clock_gating(struct intel_digital_port *dig_port, bool enable) MG_DP_MODE_CFG_GAONPWR_GATING; for (ln = 0; ln < 2; ln++) { - if (INTEL_GEN(dev_priv) >= 12) { - I915_WRITE(HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port, ln)); - val = I915_READ(DKL_DP_MODE(tc_port)); - } else { - val = I915_READ(MG_DP_MODE(ln, tc_port)); - } + val = I915_READ(MG_DP_MODE(ln, tc_port)); if (enable) val |= bits; else val &= ~bits; - if (INTEL_GEN(dev_priv) >= 12) - I915_WRITE(DKL_DP_MODE(tc_port), val); - else - I915_WRITE(MG_DP_MODE(ln, tc_port), val); + I915_WRITE(MG_DP_MODE(ln, tc_port), val); } - if (INTEL_GEN(dev_priv) == 11) { - bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | - MG_MISC_SUS0_CFG_CL2PWR_GATING | - MG_MISC_SUS0_CFG_GAONPWR_GATING | - MG_MISC_SUS0_CFG_TRPWR_GATING | - MG_MISC_SUS0_CFG_CL1PWR_GATING | - MG_MISC_SUS0_CFG_DGPWR_GATING; + bits = MG_MISC_SUS0_CFG_TR2PWR_GATING | MG_MISC_SUS0_CFG_CL2PWR_GATING | + MG_MISC_SUS0_CFG_GAONPWR_GATING | MG_MISC_SUS0_CFG_TRPWR_GATING | + MG_MISC_SUS0_CFG_CL1PWR_GATING | MG_MISC_SUS0_CFG_DGPWR_GATING; - val = I915_READ(MG_MISC_SUS0(tc_port)); - if (enable) - val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); - else - val &= ~(bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); - I915_WRITE(MG_MISC_SUS0(tc_port), val); - } + val = I915_READ(MG_MISC_SUS0(tc_port)); + if (enable) + val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3)); + else + val &= ~(bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK); + I915_WRITE(MG_MISC_SUS0(tc_port), val); } static void @@ -3508,12 +3499,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder, * down this function. */ - /* - * 7.d Type C with DP alternate or fixed/legacy/static connection - - * Disable PHY clock gating per Type-C DDI Buffer page - */ - icl_phy_set_clock_gating(dig_port, false); - /* 7.e Configure voltage swing and related IO settings */ tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level, encoder->type); @@ -3565,15 +3550,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder, if (!is_trans_port_sync_mode(crtc_state)) intel_dp_stop_link_train(intel_dp); - /* - * TODO: enable clock gating - * - * It is not written in DP enabling sequence but "PHY Clockgating - * programming" states that clock gating should be enabled after the - * link training but doing so causes all the following trainings to fail - * so not enabling it for now. - */ - /* 7.l Configure and enable FEC if needed */ intel_ddi_enable_fec(encoder, crtc_state); intel_dsc_enable(encoder, crtc_state);