[Intel-gfx] [PATCH] drm/i915/ehl: Add support for DPLL4 (v3)

[Vivek Kasireddy]

On Fri, 5 Apr 2019 17:46:38 -0700
Lucas De Marchi <lucas.demarchi at intel.com> wrote:
Hi,

> On Fri, Apr 05, 2019 at 10:59:53AM -0700, Vivek Kasireddy wrote:
> >This patch adds support for DPLL4 on EHL that include the
> >following restrictions:
> >
> >- DPLL4 cannot be used with DDIA (combo port A internal eDP usage).
> >  DPLL4 can be used with other DDIs, including DDID
> >  (combo port A external usage).
> >
> >- DPLL4 cannot be enabled when DC5 or DC6 are enabled.
> >
> >- The DPLL4 enable, lock, power enabled, and power state are
> >connected
> >  to the MGPLL1_ENABLE register.  
> 
> ok
> 
> >
> >v2: (suggestions from Bob Paauwe)
> >- Rework ehl_get_dpll() function to call intel_find_shared_dpll() and
> >  iterate twice: once for Combo plls and once for MG plls.
> >
> >- Use MG pll funcs for DPLL4 instead of creating new ones and modify
> >  mg_pll_enable to include the restrictions for EHL.  
> 
> these 2 don't match spec.
> 
> "3rd PLL for use with combo PHY (DPLL4) and 3rd combo PHY DDI clocks
> (DDIC clock)"
> 
> This is a combophy pll, not a mg phy pll. The only thing that is
> hooked to mg registers is the enable. So my understanding is that
> what you need:
> 
>   - use the dpll calculations
>   - make sure intel_find_shared_dpll doesn't this if it's for eDP
>   - setup the enable/disable to use MG_ENABLE register
Looks like my interpretation of the spec is different from yours but
your comments make sense. Should I create a new ID for this DPLL
or juse re-use DPLL_ID_ICL_MGPLL1?

> 
> >
> >v3: Fix compilation error
> >
> >Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> >Reviewed-by: Bob Paauwe <bob.j.paauwe at intel.com>
> >---
> > drivers/gpu/drm/i915/intel_dpll_mgr.c | 60
> > ++++++++++++++++++++++++++++++++++- 1 file changed, 59
> > insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >b/drivers/gpu/drm/i915/intel_dpll_mgr.c index
> >e01c057ce50b..c3f0b9720c54 100644 ---
> >a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++
> >b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2870,6 +2870,56 @@
> >icl_get_dpll(struct intel_crtc_state *crtc_state,
> > 	return pll;
> > }
> >
> >+static struct intel_shared_dpll *
> >+ehl_get_dpll(struct intel_crtc_state *crtc_state,
> >+	     struct intel_encoder *encoder)
> >+{
> >+	struct drm_i915_private *dev_priv =
> >to_i915(crtc_state->base.crtc->dev);
> >+	struct intel_shared_dpll *pll;
> >+	enum port port = encoder->port;
> >+	enum intel_dpll_id min, max;
> >+	bool ret;
> >+
> >+	if (!intel_port_is_combophy(dev_priv, port)) {
> >+		MISSING_CASE(port);
> >+		return NULL;
> >+	}
> >+
> >+	min = DPLL_ID_ICL_DPLL0;
> >+	max = DPLL_ID_ICL_DPLL1;
> >+	ret = icl_calc_dpll_state(crtc_state, encoder);
> >+	if (ret) {
> >+		pll = intel_find_shared_dpll(crtc_state, min, max);
> >+		if (pll) {
> >+			intel_reference_shared_dpll(pll,
> >crtc_state);
> >+			return pll;
> >+		}
> >+	} else {
> >+		DRM_DEBUG_KMS("Could not calculate PLL state.\n");  
> 
> the check for ret is swapped and you are missing a return here.
Unless I am reading it utterly wrong, icl_get_dpll has this:
if (!ret) {
                DRM_DEBUG_KMS("Could not calculate PLL state.\n");
                return NULL;

> 
> But given the comments above, I think it would be better to reuse
> icl_get_dpll() rather than what you are doing here.
I could have used icl_get_dpll() but thought it would be much cleaner
to have a separate function for EHL; otherwise, I guess I need to
sprinkle icl_get_dpll with many if(EHL) statements.

> 
> >+	}
> >+
> >+	if (encoder->type == INTEL_OUTPUT_EDP) {
> >+		DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n");
> >+		return NULL;
> >+	}  
> 
> this is already too late
The idea was if we have EDP being used, then we first try to find if
one of the combo PHY DPLLs are available to be used. If they are
not, then we come here and return as we cannot use this one either.

> 
> >+
> >+	min = max = DPLL_ID_ICL_MGPLL1;
> >+	ret = icl_calc_mg_pll_state(crtc_state);
> >+	if (!ret) {
> >+		DRM_DEBUG_KMS("Could not calculate PLL state.\n");
> >+		return NULL;  
> 
> again... ret == 0 is success, not otherwise
I'll send out a v4 with your suggestions soon.

Thanks,
Vivek
> 
> Lucas De Marchi
> 
> >+	}
> >+
> >+	pll = intel_find_shared_dpll(crtc_state, min, max);
> >+	if (!pll) {
> >+		DRM_DEBUG_KMS("No PLL selected\n");
> >+		return NULL;
> >+	}
> >+
> >+	intel_reference_shared_dpll(pll, crtc_state);
> >+	return pll;
> >+}
> >+
> > static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> > 				struct intel_shared_dpll *pll,
> > 				struct intel_dpll_hw_state
> > *hw_state)
> >@@ -3115,6 +3165,13 @@ static void mg_pll_enable(struct
> >drm_i915_private *dev_priv,
> > 	i915_reg_t enable_reg =
> > 		MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id));
> >
> >+	if (IS_ELKHARTLAKE(dev_priv) &&
> >+	   (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5 ||
> >+	    I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6)) {
> >+		DRM_ERROR("Cant enable DPLL4 when DC5 or DC6 are
> >enabled\n");
> >+		return;
> >+	}
> >+
> > 	icl_pll_power_enable(dev_priv, pll, enable_reg);
> >
> > 	icl_mg_pll_write(dev_priv, pll);
> >@@ -3249,12 +3306,13 @@ static const struct intel_dpll_mgr
> >icl_pll_mgr = {
> > static const struct dpll_info ehl_plls[] = {
> > 	{ "DPLL 0", &combo_pll_funcs, DPLL_ID_ICL_DPLL0, 0 },
> > 	{ "DPLL 1", &combo_pll_funcs, DPLL_ID_ICL_DPLL1, 0 },
> >+	{ "DPLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1,0 },
> > 	{ },
> > };
> >
> > static const struct intel_dpll_mgr ehl_pll_mgr = {
> > 	.dpll_info = ehl_plls,
> >-	.get_dpll = icl_get_dpll,
> >+	.get_dpll = ehl_get_dpll,
> > 	.dump_hw_state = icl_dump_hw_state,
> > };
> >
> >-- 
> >2.14.5
> >