| Message ID | 1491506163-14587-28-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 12:15:24PM -0700, Rodrigo Vivi wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Add support for reading out the cdclk frequency from the hardware on > CNL. Very similar to BXT, with a few new twists and turns: > * the PLL is now called CDCLK PLL, not DE PLL > * reference clock can be 24 MHz in addition to the 19.2 MHz BXT had > * the ratio now lives in the PLL enable register > * Only 1x and 2x CD2X dividers are supported > > v2: Deal with PLL lock bit the same way as BXT/SKL do now > v3: DSSM refclk indicator is bit 31 not 24 (Ander) > v4: Rebased by Rodrigo after Ville's cdclk rework. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 ++++ > drivers/gpu/drm/i915/intel_cdclk.c | 54 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index ac8a223..8353892 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6557,6 +6557,9 @@ enum { > #define SKL_DFSM_PIPE_B_DISABLE (1 << 21) > #define SKL_DFSM_PIPE_C_DISABLE (1 << 28) > > +#define SKL_DSSM _MMIO(0x51004) > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) > + > #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14) > > @@ -8130,6 +8133,8 @@ enum { > #define BXT_DE_PLL_ENABLE _MMIO(0x46070) > #define BXT_DE_PLL_PLL_ENABLE (1 << 31) > #define BXT_DE_PLL_LOCK (1 << 30) > +#define CNL_CDCLK_PLL_RATIO(x) (x) /* {28,44} * 19.2 or 24MHz */ Nit: 35,55 are also valid for 19.2MHz. > +#define CNL_CDCLK_PLL_RATIO_MASK 0xff > > /* GEN9 DC */ > #define DC_STATE_EN _MMIO(0x45504) > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index 4745596..a4e2bd5 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1400,6 +1400,56 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) > bxt_set_cdclk(dev_priv, &cdclk_state); > } > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > + struct intel_cdclk_state *cdclk_state) > +{ > + u32 val; > + > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > + cdclk_state->ref = 24000; > + else > + cdclk_state->ref = 19200; > + > + cdclk_state->vco = 0; > + > + val = I915_READ(BXT_DE_PLL_ENABLE); > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > + return; > + > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > + return; > + > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > +} > + > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > + struct intel_cdclk_state *cdclk_state) > +{ > + u32 divider; > + int div; > + > + cnl_cdclk_pll_update(dev_priv, cdclk_state); The other platforms set cdclk to the ref clock here, not sure if it's ok to leave it uninited. With that change it looks ok: Reviewed-by: Imre Deak <imre.deak@intel.com> > + > + if (cdclk_state->vco == 0) > + return; > + > + divider = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; > + > + switch (divider) { > + case BXT_CDCLK_CD2X_DIV_SEL_1: > + div = 2; > + break; > + case BXT_CDCLK_CD2X_DIV_SEL_2: > + div = 4; > + break; > + default: > + MISSING_CASE(divider); > + return; > + } > + > + cdclk_state->cdclk = DIV_ROUND_CLOSEST(cdclk_state->vco, div); > +} > + > /** > * intel_cdclk_state_compare - Determine if two CDCLK states differ > * @a: first CDCLK state > @@ -1897,7 +1947,9 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > skl_modeset_calc_cdclk; > } > > - if (IS_GEN9_BC(dev_priv)) > + if (IS_CANNONLAKE(dev_priv)) > + dev_priv->display.get_cdclk = cnl_get_cdclk; > + else if (IS_GEN9_BC(dev_priv)) > dev_priv->display.get_cdclk = skl_get_cdclk; > else if (IS_GEN9_LP(dev_priv)) > dev_priv->display.get_cdclk = bxt_get_cdclk; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > On Thu, Apr 06, 2017 at 12:15:24PM -0700, Rodrigo Vivi wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Add support for reading out the cdclk frequency from the hardware on > > CNL. Very similar to BXT, with a few new twists and turns: > > * the PLL is now called CDCLK PLL, not DE PLL > > * reference clock can be 24 MHz in addition to the 19.2 MHz BXT had > > * the ratio now lives in the PLL enable register > > * Only 1x and 2x CD2X dividers are supported > > > > v2: Deal with PLL lock bit the same way as BXT/SKL do now > > v3: DSSM refclk indicator is bit 31 not 24 (Ander) > > v4: Rebased by Rodrigo after Ville's cdclk rework. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 5 ++++ > > drivers/gpu/drm/i915/intel_cdclk.c | 54 +++++++++++++++++++++++++++++++++++++- > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index ac8a223..8353892 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6557,6 +6557,9 @@ enum { > > #define SKL_DFSM_PIPE_B_DISABLE (1 << 21) > > #define SKL_DFSM_PIPE_C_DISABLE (1 << 28) > > > > +#define SKL_DSSM _MMIO(0x51004) > > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) > > + > > #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) > > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14) > > > > @@ -8130,6 +8133,8 @@ enum { > > #define BXT_DE_PLL_ENABLE _MMIO(0x46070) > > #define BXT_DE_PLL_PLL_ENABLE (1 << 31) > > #define BXT_DE_PLL_LOCK (1 << 30) > > +#define CNL_CDCLK_PLL_RATIO(x) (x) /* {28,44} * 19.2 or 24MHz */ > > Nit: 35,55 are also valid for 19.2MHz. hm... should I just remove the comments then?! > > +#define CNL_CDCLK_PLL_RATIO_MASK 0xff > > > > /* GEN9 DC */ > > #define DC_STATE_EN _MMIO(0x45504) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > > index 4745596..a4e2bd5 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1400,6 +1400,56 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) > > bxt_set_cdclk(dev_priv, &cdclk_state); > > } > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > + struct intel_cdclk_state *cdclk_state) > > +{ > > + u32 val; > > + > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > + cdclk_state->ref = 24000; > > + else > > + cdclk_state->ref = 19200; > > + > > + cdclk_state->vco = 0; > > + > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > + return; > > + > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > + return; > > + > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > +} > > + > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > + struct intel_cdclk_state *cdclk_state) > > +{ > > + u32 divider; > > + int div; > > + > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > The other platforms set cdclk to the ref clock here, not sure > if it's ok to leave it uninited. With that change it looks ok: Not sure how to address this here... I see bxt and skl using the cdclk_state here... > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > > + > > + if (cdclk_state->vco == 0) > > + return; > > + > > + divider = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; > > + > > + switch (divider) { > > + case BXT_CDCLK_CD2X_DIV_SEL_1: > > + div = 2; > > + break; > > + case BXT_CDCLK_CD2X_DIV_SEL_2: > > + div = 4; > > + break; > > + default: > > + MISSING_CASE(divider); > > + return; > > + } > > + > > + cdclk_state->cdclk = DIV_ROUND_CLOSEST(cdclk_state->vco, div); > > +} > > + > > /** > > * intel_cdclk_state_compare - Determine if two CDCLK states differ > > * @a: first CDCLK state > > @@ -1897,7 +1947,9 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > > skl_modeset_calc_cdclk; > > } > > > > - if (IS_GEN9_BC(dev_priv)) > > + if (IS_CANNONLAKE(dev_priv)) > > + dev_priv->display.get_cdclk = cnl_get_cdclk; > > + else if (IS_GEN9_BC(dev_priv)) > > dev_priv->display.get_cdclk = skl_get_cdclk; > > else if (IS_GEN9_LP(dev_priv)) > > dev_priv->display.get_cdclk = bxt_get_cdclk; > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > > On Thu, Apr 06, 2017 at 12:15:24PM -0700, Rodrigo Vivi wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Add support for reading out the cdclk frequency from the hardware on > > > CNL. Very similar to BXT, with a few new twists and turns: > > > * the PLL is now called CDCLK PLL, not DE PLL > > > * reference clock can be 24 MHz in addition to the 19.2 MHz BXT had > > > * the ratio now lives in the PLL enable register > > > * Only 1x and 2x CD2X dividers are supported > > > > > > v2: Deal with PLL lock bit the same way as BXT/SKL do now > > > v3: DSSM refclk indicator is bit 31 not 24 (Ander) > > > v4: Rebased by Rodrigo after Ville's cdclk rework. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 5 ++++ > > > drivers/gpu/drm/i915/intel_cdclk.c | 54 +++++++++++++++++++++++++++++++++++++- > > > 2 files changed, 58 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index ac8a223..8353892 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -6557,6 +6557,9 @@ enum { > > > #define SKL_DFSM_PIPE_B_DISABLE (1 << 21) > > > #define SKL_DFSM_PIPE_C_DISABLE (1 << 28) > > > > > > +#define SKL_DSSM _MMIO(0x51004) > > > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) > > > + > > > #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) > > > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14) > > > > > > @@ -8130,6 +8133,8 @@ enum { > > > #define BXT_DE_PLL_ENABLE _MMIO(0x46070) > > > #define BXT_DE_PLL_PLL_ENABLE (1 << 31) > > > #define BXT_DE_PLL_LOCK (1 << 30) > > > +#define CNL_CDCLK_PLL_RATIO(x) (x) /* {28,44} * 19.2 or 24MHz */ > > > > > Nit: 35,55 are also valid for 19.2MHz. > > hm... should I just remove the comments then?! > > > > > +#define CNL_CDCLK_PLL_RATIO_MASK 0xff > > > > > > /* GEN9 DC */ > > > #define DC_STATE_EN _MMIO(0x45504) > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > > > index 4745596..a4e2bd5 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -1400,6 +1400,56 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) > > > bxt_set_cdclk(dev_priv, &cdclk_state); > > > } > > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > > + struct intel_cdclk_state *cdclk_state) > > > +{ > > > + u32 val; > > > + > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > > + cdclk_state->ref = 24000; > > > + else > > > + cdclk_state->ref = 19200; > > > + > > > + cdclk_state->vco = 0; > > > + > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > > + return; > > > + > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > > + return; > > > + > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > > +} > > > + > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > > + struct intel_cdclk_state *cdclk_state) > > > +{ > > > + u32 divider; > > > + int div; > > > + > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > > > The other platforms set cdclk to the ref clock here, not sure > > if it's ok to leave it uninited. With that change it looks ok: > > Not sure how to address this here... > I see bxt and skl using the cdclk_state here... Assuming refclk is the bypass clock then just doing what the earlier platforms do would be correct. IIRC there was some platform where the bypass clock wasn't the refclk, but that was perhaps some future thing. Either way, whatever the bypass clock is we will want the readout to correctly reflect it when the PLL is off. > > > > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > > > > + > > > + if (cdclk_state->vco == 0) > > > + return; > > > + > > > + divider = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; > > > + > > > + switch (divider) { > > > + case BXT_CDCLK_CD2X_DIV_SEL_1: > > > + div = 2; > > > + break; > > > + case BXT_CDCLK_CD2X_DIV_SEL_2: > > > + div = 4; > > > + break; > > > + default: > > > + MISSING_CASE(divider); > > > + return; > > > + } > > > + > > > + cdclk_state->cdclk = DIV_ROUND_CLOSEST(cdclk_state->vco, div); > > > +} > > > + > > > /** > > > * intel_cdclk_state_compare - Determine if two CDCLK states differ > > > * @a: first CDCLK state > > > @@ -1897,7 +1947,9 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > > > skl_modeset_calc_cdclk; > > > } > > > > > > - if (IS_GEN9_BC(dev_priv)) > > > + if (IS_CANNONLAKE(dev_priv)) > > > + dev_priv->display.get_cdclk = cnl_get_cdclk; > > > + else if (IS_GEN9_BC(dev_priv)) > > > dev_priv->display.get_cdclk = skl_get_cdclk; > > > else if (IS_GEN9_LP(dev_priv)) > > > dev_priv->display.get_cdclk = bxt_get_cdclk; > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > > > > [...] > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > > > + struct intel_cdclk_state *cdclk_state) > > > > +{ > > > > + u32 val; > > > > + > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > > > + cdclk_state->ref = 24000; > > > > + else > > > > + cdclk_state->ref = 19200; > > > > + > > > > + cdclk_state->vco = 0; > > > > + > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > > > + return; > > > > + > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > > > + return; > > > > + > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > > > +} > > > > + > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > > > + struct intel_cdclk_state *cdclk_state) > > > > +{ > > > > + u32 divider; > > > > + int div; > > > > + > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > > > > > The other platforms set cdclk to the ref clock here, not sure > > > if it's ok to leave it uninited. With that change it looks ok: > > > > Not sure how to address this here... > > I see bxt and skl using the cdclk_state here... > > Assuming refclk is the bypass clock then just doing what the earlier > platforms do would be correct. Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL is disabled. > IIRC there was some platform where the bypass clock wasn't the refclk, > but that was perhaps some future thing. Either way, whatever the > bypass clock is we will want the readout to correctly reflect it when > the PLL is off.
On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote: > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > > > > > [...] > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > > > > + struct intel_cdclk_state *cdclk_state) > > > > > +{ > > > > > + u32 val; > > > > > + > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > > > > + cdclk_state->ref = 24000; > > > > > + else > > > > > + cdclk_state->ref = 19200; > > > > > + > > > > > + cdclk_state->vco = 0; > > > > > + > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > > > > + return; > > > > > + > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > > > > + return; > > > > > + > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > > > > +} > > > > > + > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > > > > + struct intel_cdclk_state *cdclk_state) > > > > > +{ > > > > > + u32 divider; > > > > > + int div; > > > > > + > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > > > > > > > The other platforms set cdclk to the ref clock here, not sure > > > > if it's ok to leave it uninited. With that change it looks ok: > > > > > > Not sure how to address this here... > > > I see bxt and skl using the cdclk_state here... > > > > Assuming refclk is the bypass clock then just doing what the earlier > > platforms do would be correct. > > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL > is disabled. So, do I need to change anything? > > > IIRC there was some platform where the bypass clock wasn't the refclk, > > but that was perhaps some future thing. Either way, whatever the > > bypass clock is we will want the readout to correctly reflect it when > > the PLL is off.
On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote: > On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote: > > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: > > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > > > > > > [...] > > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > > > > > + struct intel_cdclk_state *cdclk_state) > > > > > > +{ > > > > > > + u32 val; > > > > > > + > > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > > > > > + cdclk_state->ref = 24000; > > > > > > + else > > > > > > + cdclk_state->ref = 19200; > > > > > > + > > > > > > + cdclk_state->vco = 0; > > > > > > + > > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > > > > > + return; > > > > > > + > > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > > > > > + return; > > > > > > + > > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > > > > > +} > > > > > > + > > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > > > > > + struct intel_cdclk_state *cdclk_state) > > > > > > +{ > > > > > > + u32 divider; > > > > > > + int div; > > > > > > + > > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > > > > > > > > > The other platforms set cdclk to the ref clock here, not sure > > > > > if it's ok to leave it uninited. With that change it looks ok: > > > > > > > > Not sure how to address this here... > > > > I see bxt and skl using the cdclk_state here... > > > > > > Assuming refclk is the bypass clock then just doing what the earlier > > > platforms do would be correct. > > > > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL > > is disabled. > > So, do I need to change anything? Yes, add the following after cnl_cdclk_pll_update() as done on other gen9+ platforms: cdclk_state->cdclk = cdclk_state->ref; --Imre > > > > > > IIRC there was some platform where the bypass clock wasn't the refclk, > > > but that was perhaps some future thing. Either way, whatever the > > > bypass clock is we will want the readout to correctly reflect it when > > > the PLL is off. >
When addressing Imre's comments I noticed:
error: ‘cnl_set_cdclk’ defined but not used [-Werror=unused-function]
static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
^
cc1: all warnings being treated as errors
Ville, since the original is yours I'd like your advise on how to proceed.
1. squash both
2. swap the patches and create a temporary emply cnl_set_cdclk
3. ?
Thanks,
Rodrigo.
On Mon, Jun 5, 2017 at 1:07 PM, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote:
>> On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote:
>> > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote:
>> > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote:
>> > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote:
>> > > > > > [...]
>> > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv,
>> > > > > > + struct intel_cdclk_state *cdclk_state)
>> > > > > > +{
>> > > > > > + u32 val;
>> > > > > > +
>> > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz)
>> > > > > > + cdclk_state->ref = 24000;
>> > > > > > + else
>> > > > > > + cdclk_state->ref = 19200;
>> > > > > > +
>> > > > > > + cdclk_state->vco = 0;
>> > > > > > +
>> > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE);
>> > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
>> > > > > > + return;
>> > > > > > +
>> > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
>> > > > > > + return;
>> > > > > > +
>> > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref;
>> > > > > > +}
>> > > > > > +
>> > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv,
>> > > > > > + struct intel_cdclk_state *cdclk_state)
>> > > > > > +{
>> > > > > > + u32 divider;
>> > > > > > + int div;
>> > > > > > +
>> > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state);
>> > > > >
>> > > > > The other platforms set cdclk to the ref clock here, not sure
>> > > > > if it's ok to leave it uninited. With that change it looks ok:
>> > > >
>> > > > Not sure how to address this here...
>> > > > I see bxt and skl using the cdclk_state here...
>> > >
>> > > Assuming refclk is the bypass clock then just doing what the earlier
>> > > platforms do would be correct.
>> >
>> > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL
>> > is disabled.
>>
>> So, do I need to change anything?
>
> Yes, add the following after cnl_cdclk_pll_update() as done on other
> gen9+ platforms:
>
> cdclk_state->cdclk = cdclk_state->ref;
>
> --Imre
>
>>
>> >
>> > > IIRC there was some platform where the bypass clock wasn't the refclk,
>> > > but that was perhaps some future thing. Either way, whatever the
>> > > bypass clock is we will want the readout to correctly reflect it when
>> > > the PLL is off.
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 06, 2017 at 02:56:23PM -0700, Rodrigo Vivi wrote: > When addressing Imre's comments I noticed: > > error: ‘cnl_set_cdclk’ defined but not used [-Werror=unused-function] > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > ^ > cc1: all warnings being treated as errors > > Ville, since the original is yours I'd like your advise on how to proceed. > 1. squash both > 2. swap the patches and create a temporary emply cnl_set_cdclk > 3. ? I would just ignore that since it gets fixed in the following patches. I know adding unused stuff isn't really looked upon favorably but squashing it with the display init patch would make that patch less focused and quite large. I guess you could squash it with the dynamic cdclk change patch, but then you'd have to reorder the display init patch to be after that and that order doesn't make as much sense since you would then be adding optional extra functionality before the basic functionality in in place. > > Thanks, > Rodrigo. > > On Mon, Jun 5, 2017 at 1:07 PM, Imre Deak <imre.deak@intel.com> wrote: > > On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote: > >> On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote: > >> > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: > >> > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > >> > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > >> > > > > > [...] > >> > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > >> > > > > > + struct intel_cdclk_state *cdclk_state) > >> > > > > > +{ > >> > > > > > + u32 val; > >> > > > > > + > >> > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > >> > > > > > + cdclk_state->ref = 24000; > >> > > > > > + else > >> > > > > > + cdclk_state->ref = 19200; > >> > > > > > + > >> > > > > > + cdclk_state->vco = 0; > >> > > > > > + > >> > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > >> > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > >> > > > > > + return; > >> > > > > > + > >> > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > >> > > > > > + return; > >> > > > > > + > >> > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > >> > > > > > +} > >> > > > > > + > >> > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > >> > > > > > + struct intel_cdclk_state *cdclk_state) > >> > > > > > +{ > >> > > > > > + u32 divider; > >> > > > > > + int div; > >> > > > > > + > >> > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > >> > > > > > >> > > > > The other platforms set cdclk to the ref clock here, not sure > >> > > > > if it's ok to leave it uninited. With that change it looks ok: > >> > > > > >> > > > Not sure how to address this here... > >> > > > I see bxt and skl using the cdclk_state here... > >> > > > >> > > Assuming refclk is the bypass clock then just doing what the earlier > >> > > platforms do would be correct. > >> > > >> > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL > >> > is disabled. > >> > >> So, do I need to change anything? > > > > Yes, add the following after cnl_cdclk_pll_update() as done on other > > gen9+ platforms: > > > > cdclk_state->cdclk = cdclk_state->ref; > > > > --Imre > > > >> > >> > > >> > > IIRC there was some platform where the bypass clock wasn't the refclk, > >> > > but that was perhaps some future thing. Either way, whatever the > >> > > bypass clock is we will want the readout to correctly reflect it when > >> > > the PLL is off. > >> > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 07, 2017 at 01:59:05PM +0300, Ville Syrjälä wrote: > On Tue, Jun 06, 2017 at 02:56:23PM -0700, Rodrigo Vivi wrote: > > When addressing Imre's comments I noticed: > > > > error: ‘cnl_set_cdclk’ defined but not used [-Werror=unused-function] > > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > > ^ > > cc1: all warnings being treated as errors > > > > Ville, since the original is yours I'd like your advise on how to proceed. > > 1. squash both > > 2. swap the patches and create a temporary emply cnl_set_cdclk > > 3. ? > > I would just ignore that since it gets fixed in the following patches. > I know adding unused stuff isn't really looked upon favorably but > squashing it with the display init patch would make that patch less > focused and quite large. I guess you could squash it with the > dynamic cdclk change patch, but then you'd have to reorder the display > init patch to be after that and that order doesn't make as much sense > since you would then be adding optional extra functionality before the > basic functionality in in place. If you really want to avoid the warning then I guess adding __attribute__((unused)) and subsequently removing it would do the trick. > > > > > Thanks, > > Rodrigo. > > > > On Mon, Jun 5, 2017 at 1:07 PM, Imre Deak <imre.deak@intel.com> wrote: > > > On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote: > > >> On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote: > > >> > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: > > >> > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: > > >> > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: > > >> > > > > > [...] > > >> > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, > > >> > > > > > + struct intel_cdclk_state *cdclk_state) > > >> > > > > > +{ > > >> > > > > > + u32 val; > > >> > > > > > + > > >> > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) > > >> > > > > > + cdclk_state->ref = 24000; > > >> > > > > > + else > > >> > > > > > + cdclk_state->ref = 19200; > > >> > > > > > + > > >> > > > > > + cdclk_state->vco = 0; > > >> > > > > > + > > >> > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); > > >> > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) > > >> > > > > > + return; > > >> > > > > > + > > >> > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) > > >> > > > > > + return; > > >> > > > > > + > > >> > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; > > >> > > > > > +} > > >> > > > > > + > > >> > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, > > >> > > > > > + struct intel_cdclk_state *cdclk_state) > > >> > > > > > +{ > > >> > > > > > + u32 divider; > > >> > > > > > + int div; > > >> > > > > > + > > >> > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); > > >> > > > > > > >> > > > > The other platforms set cdclk to the ref clock here, not sure > > >> > > > > if it's ok to leave it uninited. With that change it looks ok: > > >> > > > > > >> > > > Not sure how to address this here... > > >> > > > I see bxt and skl using the cdclk_state here... > > >> > > > > >> > > Assuming refclk is the bypass clock then just doing what the earlier > > >> > > platforms do would be correct. > > >> > > > >> > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL > > >> > is disabled. > > >> > > >> So, do I need to change anything? > > > > > > Yes, add the following after cnl_cdclk_pll_update() as done on other > > > gen9+ platforms: > > > > > > cdclk_state->cdclk = cdclk_state->ref; > > > > > > --Imre > > > > > >> > > >> > > > >> > > IIRC there was some platform where the bypass clock wasn't the refclk, > > >> > > but that was perhaps some future thing. Either way, whatever the > > >> > > bypass clock is we will want the readout to correctly reflect it when > > >> > > the PLL is off. > > >> > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > Rodrigo Vivi > > Blog: http://blog.vivi.eng.br > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 7, 2017 at 4:09 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jun 07, 2017 at 01:59:05PM +0300, Ville Syrjälä wrote: >> On Tue, Jun 06, 2017 at 02:56:23PM -0700, Rodrigo Vivi wrote: >> > When addressing Imre's comments I noticed: >> > >> > error: ‘cnl_set_cdclk’ defined but not used [-Werror=unused-function] >> > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, >> > ^ >> > cc1: all warnings being treated as errors >> > >> > Ville, since the original is yours I'd like your advise on how to proceed. >> > 1. squash both >> > 2. swap the patches and create a temporary emply cnl_set_cdclk >> > 3. ? >> >> I would just ignore that since it gets fixed in the following patches. >> I know adding unused stuff isn't really looked upon favorably but >> squashing it with the display init patch would make that patch less >> focused and quite large. I guess you could squash it with the >> dynamic cdclk change patch, but then you'd have to reorder the display >> init patch to be after that and that order doesn't make as much sense >> since you would then be adding optional extra functionality before the >> basic functionality in in place. > > If you really want to avoid the warning then I guess adding > __attribute__((unused)) and subsequently removing it would do the trick. cool! I liked that trick. So whenever someone is bisecting and end up on that patch the compilation is not broken with the Werror enabled. ;) > >> >> > >> > Thanks, >> > Rodrigo. >> > >> > On Mon, Jun 5, 2017 at 1:07 PM, Imre Deak <imre.deak@intel.com> wrote: >> > > On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote: >> > >> On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote: >> > >> > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote: >> > >> > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote: >> > >> > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote: >> > >> > > > > > [...] >> > >> > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, >> > >> > > > > > + struct intel_cdclk_state *cdclk_state) >> > >> > > > > > +{ >> > >> > > > > > + u32 val; >> > >> > > > > > + >> > >> > > > > > + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) >> > >> > > > > > + cdclk_state->ref = 24000; >> > >> > > > > > + else >> > >> > > > > > + cdclk_state->ref = 19200; >> > >> > > > > > + >> > >> > > > > > + cdclk_state->vco = 0; >> > >> > > > > > + >> > >> > > > > > + val = I915_READ(BXT_DE_PLL_ENABLE); >> > >> > > > > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) >> > >> > > > > > + return; >> > >> > > > > > + >> > >> > > > > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) >> > >> > > > > > + return; >> > >> > > > > > + >> > >> > > > > > + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; >> > >> > > > > > +} >> > >> > > > > > + >> > >> > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, >> > >> > > > > > + struct intel_cdclk_state *cdclk_state) >> > >> > > > > > +{ >> > >> > > > > > + u32 divider; >> > >> > > > > > + int div; >> > >> > > > > > + >> > >> > > > > > + cnl_cdclk_pll_update(dev_priv, cdclk_state); >> > >> > > > > >> > >> > > > > The other platforms set cdclk to the ref clock here, not sure >> > >> > > > > if it's ok to leave it uninited. With that change it looks ok: >> > >> > > > >> > >> > > > Not sure how to address this here... >> > >> > > > I see bxt and skl using the cdclk_state here... >> > >> > > >> > >> > > Assuming refclk is the bypass clock then just doing what the earlier >> > >> > > platforms do would be correct. >> > >> > >> > >> > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL >> > >> > is disabled. >> > >> >> > >> So, do I need to change anything? >> > > >> > > Yes, add the following after cnl_cdclk_pll_update() as done on other >> > > gen9+ platforms: >> > > >> > > cdclk_state->cdclk = cdclk_state->ref; >> > > >> > > --Imre >> > > >> > >> >> > >> > >> > >> > > IIRC there was some platform where the bypass clock wasn't the refclk, >> > >> > > but that was perhaps some future thing. Either way, whatever the >> > >> > > bypass clock is we will want the readout to correctly reflect it when >> > >> > > the PLL is off. >> > >> >> > > _______________________________________________ >> > > Intel-gfx mailing list >> > > Intel-gfx@lists.freedesktop.org >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> > >> > >> > -- >> > Rodrigo Vivi >> > Blog: http://blog.vivi.eng.br >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Ville Syrjälä >> Intel OTC >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ac8a223..8353892 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6557,6 +6557,9 @@ enum { #define SKL_DFSM_PIPE_B_DISABLE (1 << 21) #define SKL_DFSM_PIPE_C_DISABLE (1 << 28) +#define SKL_DSSM _MMIO(0x51004) +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) + #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14) @@ -8130,6 +8133,8 @@ enum { #define BXT_DE_PLL_ENABLE _MMIO(0x46070) #define BXT_DE_PLL_PLL_ENABLE (1 << 31) #define BXT_DE_PLL_LOCK (1 << 30) +#define CNL_CDCLK_PLL_RATIO(x) (x) /* {28,44} * 19.2 or 24MHz */ +#define CNL_CDCLK_PLL_RATIO_MASK 0xff /* GEN9 DC */ #define DC_STATE_EN _MMIO(0x45504) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 4745596..a4e2bd5 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -1400,6 +1400,56 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) bxt_set_cdclk(dev_priv, &cdclk_state); } +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv, + struct intel_cdclk_state *cdclk_state) +{ + u32 val; + + if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz) + cdclk_state->ref = 24000; + else + cdclk_state->ref = 19200; + + cdclk_state->vco = 0; + + val = I915_READ(BXT_DE_PLL_ENABLE); + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) + return; + + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0)) + return; + + cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref; +} + +static void cnl_get_cdclk(struct drm_i915_private *dev_priv, + struct intel_cdclk_state *cdclk_state) +{ + u32 divider; + int div; + + cnl_cdclk_pll_update(dev_priv, cdclk_state); + + if (cdclk_state->vco == 0) + return; + + divider = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; + + switch (divider) { + case BXT_CDCLK_CD2X_DIV_SEL_1: + div = 2; + break; + case BXT_CDCLK_CD2X_DIV_SEL_2: + div = 4; + break; + default: + MISSING_CASE(divider); + return; + } + + cdclk_state->cdclk = DIV_ROUND_CLOSEST(cdclk_state->vco, div); +} + /** * intel_cdclk_state_compare - Determine if two CDCLK states differ * @a: first CDCLK state @@ -1897,7 +1947,9 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) skl_modeset_calc_cdclk; } - if (IS_GEN9_BC(dev_priv)) + if (IS_CANNONLAKE(dev_priv)) + dev_priv->display.get_cdclk = cnl_get_cdclk; + else if (IS_GEN9_BC(dev_priv)) dev_priv->display.get_cdclk = skl_get_cdclk; else if (IS_GEN9_LP(dev_priv)) dev_priv->display.get_cdclk = bxt_get_cdclk;