Re: [PATCH 3/5] drm/i915/ehl: Don't program PHY_MISC on EHL PHY C

Souza, Jose
2019-06-21T20:34:14Z

On Thu, 2019-06-20 at 19:01 -0700, Matt Roper wrote:
> Although EHL added a third combo PHY, no PHY_MISC register was added
> for
> PHY C. The bspec indicates that there's no need to program the "DE
> to
> IO Comp Pwr Down" setting for this PHY that we usually need to set in
> PHY_MISC.
>
> Bspec: 33148
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> .../gpu/drm/i915/display/intel_combo_phy.c | 53 +++++++++++++--
> ----
> 1 file changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> index 075bab2500eb..da590f1a998b 100644
> --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> @@ -183,9 +183,13 @@ static void cnl_combo_phys_uninit(struct
> drm_i915_private *dev_priv)
> static bool icl_combo_phy_enabled(struct drm_i915_private *dev_priv,
> enum port port)
> {
> - return !(I915_READ(ICL_PHY_MISC(port)) &
> - ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN) &&
> - (I915_READ(ICL_PORT_COMP_DW0(port)) & COMP_INIT);
> + /* The PHY C added by EHL has no PHY_MISC register */
> + if (port == PORT_C)
> + return I915_READ(ICL_PORT_COMP_DW0(port)) & COMP_INIT;

Please add IS_ELKHARTLAKE() to the if, this is a particularity of EHL,
future platforms will reuse this function and they have PHY_MISC.

> + else
> + return !(I915_READ(ICL_PHY_MISC(port)) &
> + ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN) &&
> + (I915_READ(ICL_PORT_COMP_DW0(port)) &
> COMP_INIT);
> }
>
> static bool icl_combo_phy_verify_state(struct drm_i915_private
> *dev_priv,
> @@ -300,18 +304,26 @@ static void icl_combo_phys_init(struct
> drm_i915_private *dev_priv)
> }
>
> /*
> - * EHL's combo PHY A can be hooked up to either an
> external
> - * display (via DDI-D) or an internal display (via DDI-
> A or
> - * the DSI DPHY). This is a motherboard design
> decision that
> - * can't be changed on the fly, so initialize the PHY's
> mux
> - * based on whether our VBT indicates the presence of
> any
> - * "internal" child devices.
> + * Although EHL adds a combo PHY C, there's no PHY_MISC
> + * register for it and no need to program the
> + * DE_IO_COMP_PWR_DOWN setting on PHY C.
> */
> - val = I915_READ(ICL_PHY_MISC(port));
> - if (IS_ELKHARTLAKE(dev_priv) && port == PORT_A)
> - val = ehl_combo_phy_a_mux(dev_priv, val);
> - val &= ~ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> - I915_WRITE(ICL_PHY_MISC(port), val);
> + if (port != PORT_C) {

Something as above here too.

Maybe something with less changes like:

if (IS_ELKHARTLAKE(dev_priv) && port == PORT_C)
goto skip_phy_misc;

> + /*
> + * EHL's combo PHY A can be hooked up to either
> an
> + * external display (via DDI-D) or an internal
> display
> + * (via DDI-A or the DSI DPHY). This is a
> motherboard
> + * design decision that can't be changed on the
> fly, so
> + * initialize the PHY's mux based on whether
> our VBT
> + * indicates the presence of any "internal"
> child
> + * devices.
> + */
> + val = I915_READ(ICL_PHY_MISC(port));
> + if (IS_ELKHARTLAKE(dev_priv) && port == PORT_A)
> + val = ehl_combo_phy_a_mux(dev_priv,
> val);
> + val &= ~ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> + I915_WRITE(ICL_PHY_MISC(port), val);
> + }
>
> cnl_set_procmon_ref_values(dev_priv, port);
>
> @@ -343,9 +355,16 @@ static void icl_combo_phys_uninit(struct
> drm_i915_private *dev_priv)
> DRM_WARN("Port %c combo PHY HW state changed
> unexpectedly\n",
> port_name(port));
>
> - val = I915_READ(ICL_PHY_MISC(port));
> - val |= ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> - I915_WRITE(ICL_PHY_MISC(port), val);
> + /*
> + * Although EHL adds a combo PHY C, there's no PHY_MISC
> + * register for it and no need to program the
> + * DE_IO_COMP_PWR_DOWN setting on PHY C.
> + */
> + if (port != PORT_C) {
> + val = I915_READ(ICL_PHY_MISC(port));
> + val |= ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> + I915_WRITE(ICL_PHY_MISC(port), val);
> + }

Same as above.

>
> val = I915_READ(ICL_PORT_COMP_DW0(port));
> val &= ~COMP_INIT;