Re: [PATCH 10/24] drm/i915/icl: add icelake_get_ddi_pll()

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2018-06-13 às 16:15 -0700, Lucas De Marchi escreveu:
> On Mon, May 21, 2018 at 05:25:44PM -0700, Paulo Zanoni wrote:
> > Implement the hardware state readout code.
> > 
> > Thanks to Animesh Manna for spotting this problem.
> > 
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Credits-to: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 42
> > +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 64593b0fbebd..d5a19c1b3b20 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9146,6 +9146,44 @@ static void cannonlake_get_ddi_pll(struct
> > drm_i915_private *dev_priv,
> >  	pipe_config->shared_dpll =
> > intel_get_shared_dpll_by_id(dev_priv, id);
> >  }
> >  
> > +static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> > +				enum port port,
> > +				struct intel_crtc_state
> > *pipe_config)
> > +{
> > +	enum intel_dpll_id id;
> > +	u32 temp;
> > +
> > +	/* TODO: TBT pll not implemented. */
> > +	switch (port) {
> > +	case PORT_A:
> > +	case PORT_B:
> > +		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> > +		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> > +		id = temp >>
> > DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> 
> This could be simpler:
> 
> 	temp = I915_READ(DPCLKA_CFGCR0_ICL) >>
> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> 	id = temp & DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(0);

I fail to understand why this is simpler... The same operations, just
on a different order.


> 
> But this ship has sailed, aka MASK above requires the port and
> hardcoding 0 doesn't make it better IMO.
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 

Thanks!

> 
> Lucas De Marchi
> 
> > +
> > +		if (WARN_ON(id != DPLL_ID_ICL_DPLL0 && id !=
> > DPLL_ID_ICL_DPLL1))
> > +			return;
> > +		break;
> > +	case PORT_C:
> > +		id = DPLL_ID_ICL_MGPLL1;
> > +		break;
> > +	case PORT_D:
> > +		id = DPLL_ID_ICL_MGPLL2;
> > +		break;
> > +	case PORT_E:
> > +		id = DPLL_ID_ICL_MGPLL3;
> > +		break;
> > +	case PORT_F:
> > +		id = DPLL_ID_ICL_MGPLL4;
> > +		break;
> > +	default:
> > +		MISSING_CASE(port);
> > +		return;
> > +	}
> > +
> > +	pipe_config->shared_dpll =
> > intel_get_shared_dpll_by_id(dev_priv, id);
> > +}
> > +
> >  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> >  				enum port port,
> >  				struct intel_crtc_state
> > *pipe_config)
> > @@ -9333,7 +9371,9 @@ static void haswell_get_ddi_port_state(struct
> > intel_crtc *crtc,
> >  
> >  	port = (tmp & TRANS_DDI_PORT_MASK) >>
> > TRANS_DDI_PORT_SHIFT;
> >  
> > -	if (IS_CANNONLAKE(dev_priv))
> > +	if (IS_ICELAKE(dev_priv))
> > +		icelake_get_ddi_pll(dev_priv, port, pipe_config);
> > +	else if (IS_CANNONLAKE(dev_priv))
> >  		cannonlake_get_ddi_pll(dev_priv, port,
> > pipe_config);
> >  	else if (IS_GEN9_BC(dev_priv))
> >  		skylake_get_ddi_pll(dev_priv, port, pipe_config);
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx