[Intel-gfx] [PATCH 10/24] drm/i915/icl: add icelake_get_ddi_pll()

[Lucas De Marchi]

On Wed, Jun 13, 2018 at 04:51:57PM -0700, Paulo Zanoni wrote:
> 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 at intel.com>
> > > Credits-to: Animesh Manna <animesh.manna at intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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.

If you open up the macros you will see there are more operations on what
we are doing right now... and we wouldn't need to depend on the port for
this if we had done this way. However we already depend on having the
port as an argument in other places, so there's nothing to do
differently on this particular patch.

Lucas De Marchi

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