[Intel-gfx] [PATCH 28/67] drm/i915/cnl: Implement .get_display_clock_speed() for CNL

[Ville Syrjälä]

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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC