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

[Lucas De Marchi]

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

>
>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.

But given the comments above, I think it would be better to reuse
icl_get_dpll() rather than what you are doing here.

>+	}
>+
>+	if (encoder->type == INTEL_OUTPUT_EDP) {
>+		DRM_DEBUG_KMS("Cannot use DPLL4 with EDP.\n");
>+		return NULL;
>+	}

this is already too late

>+
>+	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

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
>