Re: [PATCH 4/7] drm/i915/cnl: Fix wrpll math for higher freqs.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 14, 2017 at 12:09:39PM -0800, Rodrigo Vivi wrote:
> On Tue, Nov 14, 2017 at 08:00:31PM +0000, Ville Syrjälä wrote:
> > On Tue, Nov 14, 2017 at 11:47:56AM -0800, Rodrigo Vivi wrote:
> > > Spec describe all values in MHz. We handle our
> > > clocks in KHz. This includes the best_dco_centrality that was
> > > forgot in the same unity as spec. Consequently we couldn't
> > > get a good divider for high frequenies. Hence HDMI 2.0 wasn't
> > > working.
> > > 
> > > This patch also replaces the use of "* KHz(1)" with the values
> > > directly on KHz to avoid future confusion.
> > > 
> > > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: James Ausmus <james.ausmus@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index fba969cbda37..53f650f56148 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2201,8 +2201,8 @@ cnl_ddi_calculate_wrpll(int clock,
> > >  			struct skl_wrpll_params *wrpll_params)
> > >  {
> > >  	u32 afe_clock = clock * 5;
> > > -	u32 dco_min = 7998 * KHz(1);
> > > -	u32 dco_max = 10000 * KHz(1);
> > > +	u32 dco_min = 7998000;
> > > +	u32 dco_max = 10000000;
> > >  	u32 dco_mid = (dco_min + dco_max) / 2;
> > >  	static const int dividers[] = {  2,  4,  6,  8, 10, 12,  14,  16,
> > >  					 18, 20, 24, 28, 30, 32,  36,  40,
> > > @@ -2211,7 +2211,7 @@ cnl_ddi_calculate_wrpll(int clock,
> > >  					 84, 88, 90, 92, 96, 98, 100, 102,
> > >  					  3,  5,  7,  9, 15, 21 };
> > >  	u32 dco, best_dco = 0, dco_centrality = 0;
> > > -	u32 best_dco_centrality = 999999;
> > > +	u32 best_dco_centrality = 999999000;
> > 
> > UINT_MAX, -1, or ~0 maybe?
> 
> yeap, I considered the max macros, but I didn't want to deviate
> from spec...

Always bothers me to see some kind of 999... decimal max value pulled
out from someone's hat when they obvious thing clearly is just "max
representable value". Feels like someone wasn't thinking 100% when
they wrote the spec :)

Maybe just extend the 9's all the way to the end then? Dunno. I think
I'll just move on from the DDI DPLL code (that apporach has worked
pretty well thus far ;)

> 
> > 
> > >  	int d, best_div = 0, pdiv = 0, qdiv = 0, kdiv = 0;
> > >  
> > >  	for (d = 0; d < ARRAY_SIZE(dividers); d++) {
> > > -- 
> > > 2.13.6
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx