On Wed, Jul 03, 2019 at 06:06:58PM -0700, Matt Roper wrote:
> Although the register name implies that it operates on DDI's,
> DPCLKA_CFGCR0_ICL actually needs to be programmed according to the PHY
> that's in use. I.e., when using EHL's DDI-D on combo PHY A, the bits
> described as "port A" in the bspec are what we need to set. The bspec
> clarifies:
>
> "[For EHL] DDID clock tied to DDIA clock, so DPCLKA_CFGCR0 DDIA
> Clock Select chooses the PLL for both DDIA and DDID and drives
> port A in all cases."
>
> Also, since the CNL DPCLKA_CFGCR0 bit defines are still port-based, we
> create separate ICL-specific defines that accept the PHY rather than
> trying to share the same bit definitions between CNL and ICL.
>
> v5: Make icl_dpclka_cfgcr0_clk_off() take phy rather than port. When
> splitting the original patch the hunk to handle this wound up too
> late in the series. (Sparse)
>
> Bspec: 33148
> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
Looks correct to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 17 ++++++---
> drivers/gpu/drm/i915/display/intel_ddi.c | 47 +++++++++++++++---------
> drivers/gpu/drm/i915/i915_reg.h | 12 ++++--
> 3 files changed, 50 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index b8673debf932..f574af62888c 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -560,11 +560,13 @@ static void gen11_dsi_gate_clocks(struct intel_encoder *encoder)
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> u32 tmp;
> enum port port;
> + enum phy phy;
>
> mutex_lock(&dev_priv->dpll_lock);
> tmp = I915_READ(DPCLKA_CFGCR0_ICL);
> for_each_dsi_port(port, intel_dsi->ports) {
> - tmp |= DPCLKA_CFGCR0_DDI_CLK_OFF(port);
> + phy = intel_port_to_phy(dev_priv, port);
> + tmp |= ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
> }
>
> I915_WRITE(DPCLKA_CFGCR0_ICL, tmp);
> @@ -577,11 +579,13 @@ static void gen11_dsi_ungate_clocks(struct intel_encoder *encoder)
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> u32 tmp;
> enum port port;
> + enum phy phy;
>
> mutex_lock(&dev_priv->dpll_lock);
> tmp = I915_READ(DPCLKA_CFGCR0_ICL);
> for_each_dsi_port(port, intel_dsi->ports) {
> - tmp &= ~DPCLKA_CFGCR0_DDI_CLK_OFF(port);
> + phy = intel_port_to_phy(dev_priv, port);
> + tmp &= ~ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
> }
>
> I915_WRITE(DPCLKA_CFGCR0_ICL, tmp);
> @@ -595,19 +599,22 @@ static void gen11_dsi_map_pll(struct intel_encoder *encoder,
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> enum port port;
> + enum phy phy;
> u32 val;
>
> mutex_lock(&dev_priv->dpll_lock);
>
> val = I915_READ(DPCLKA_CFGCR0_ICL);
> for_each_dsi_port(port, intel_dsi->ports) {
> - val &= ~DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> - val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, port);
> + phy = intel_port_to_phy(dev_priv, port);
> + val &= ~ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
> + val |= ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy);
> }
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>
> for_each_dsi_port(port, intel_dsi->ports) {
> - val &= ~DPCLKA_CFGCR0_DDI_CLK_OFF(port);
> + phy = intel_port_to_phy(dev_priv, port);
> + val &= ~ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
> }
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index a4172595c8d8..065feb917db4 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2729,12 +2729,13 @@ u32 ddi_signal_levels(struct intel_dp *intel_dp)
>
> static inline
> u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
> - enum port port)
> + enum phy phy)
> {
> - if (intel_port_is_combophy(dev_priv, port)) {
> - return ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(port);
> - } else if (intel_port_is_tc(dev_priv, port)) {
> - enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> + if (intel_phy_is_combo(dev_priv, phy)) {
> + return ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
> + } else if (intel_phy_is_tc(dev_priv, phy)) {
> + enum tc_port tc_port = intel_port_to_tc(dev_priv,
> + (enum port)phy);
>
> return ICL_DPCLKA_CFGCR0_TC_CLK_OFF(tc_port);
> }
> @@ -2747,22 +2748,32 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
> {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> - enum port port = encoder->port;
> + enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> u32 val;
>
> mutex_lock(&dev_priv->dpll_lock);
>
> val = I915_READ(DPCLKA_CFGCR0_ICL);
> - WARN_ON((val & icl_dpclka_cfgcr0_clk_off(dev_priv, port)) == 0);
> + WARN_ON((val & icl_dpclka_cfgcr0_clk_off(dev_priv, phy)) == 0);
>
> - if (intel_port_is_combophy(dev_priv, port)) {
> - val &= ~DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> - val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, port);
> + if (intel_phy_is_combo(dev_priv, phy)) {
> + /*
> + * Even though this register references DDIs, note that we
> + * want to pass the PHY rather than the port (DDI). For
> + * ICL, port=phy in all cases so it doesn't matter, but for
> + * EHL the bspec notes the following:
> + *
> + * "DDID clock tied to DDIA clock, so DPCLKA_CFGCR0 DDIA
> + * Clock Select chooses the PLL for both DDIA and DDID and
> + * drives port A in all cases."
> + */
> + val &= ~ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
> + val |= ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy);
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
> POSTING_READ(DPCLKA_CFGCR0_ICL);
> }
>
> - val &= ~icl_dpclka_cfgcr0_clk_off(dev_priv, port);
> + val &= ~icl_dpclka_cfgcr0_clk_off(dev_priv, phy);
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>
> mutex_unlock(&dev_priv->dpll_lock);
> @@ -2771,13 +2782,13 @@ static void icl_map_plls_to_ports(struct intel_encoder *encoder,
> static void icl_unmap_plls_to_ports(struct intel_encoder *encoder)
> {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> - enum port port = encoder->port;
> + enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> u32 val;
>
> mutex_lock(&dev_priv->dpll_lock);
>
> val = I915_READ(DPCLKA_CFGCR0_ICL);
> - val |= icl_dpclka_cfgcr0_clk_off(dev_priv, port);
> + val |= icl_dpclka_cfgcr0_clk_off(dev_priv, phy);
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>
> mutex_unlock(&dev_priv->dpll_lock);
> @@ -2838,9 +2849,11 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
>
> val = I915_READ(DPCLKA_CFGCR0_ICL);
> for_each_port_masked(port, port_mask) {
> + enum phy phy = intel_port_to_phy(dev_priv, port);
> +
> bool ddi_clk_ungated = !(val &
> icl_dpclka_cfgcr0_clk_off(dev_priv,
> - port));
> + phy));
>
> if (ddi_clk_needed == ddi_clk_ungated)
> continue;
> @@ -2852,9 +2865,9 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
> if (WARN_ON(ddi_clk_needed))
> continue;
>
> - DRM_NOTE("Port %c is disabled/in DSI mode with an ungated DDI clock, gate it\n",
> - port_name(port));
> - val |= icl_dpclka_cfgcr0_clk_off(dev_priv, port);
> + DRM_NOTE("PHY %c is disabled/in DSI mode with an ungated DDI clock, gate it\n",
> + phy_name(port));
> + val |= icl_dpclka_cfgcr0_clk_off(dev_priv, phy);
> I915_WRITE(DPCLKA_CFGCR0_ICL, val);
> }
> }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c814cc1b3ae5..c9e2e09b6f01 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9680,17 +9680,21 @@ enum skl_power_gate {
> * CNL Clocks
> */
> #define DPCLKA_CFGCR0 _MMIO(0x6C200)
> -#define DPCLKA_CFGCR0_ICL _MMIO(0x164280)
> #define DPCLKA_CFGCR0_DDI_CLK_OFF(port) (1 << ((port) == PORT_F ? 23 : \
> (p`:wqort) + 10))
> -#define ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(port) (1 << ((port) + 10))
> -#define ICL_DPCLKA_CFGCR0_TC_CLK_OFF(tc_port) (1 << ((tc_port) == PORT_TC4 ? \
> - 21 : (tc_port) + 12))
> #define DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port) ((port) == PORT_F ? 21 : \
> (port) * 2)
> #define DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port) (3 << DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port))
> #define DPCLKA_CFGCR0_DDI_CLK_SEL(pll, port) ((pll) << DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port))
>
> +#define DPCLKA_CFGCR0_ICL _MMIO(0x164280)
> +#define ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy) (1 << _PICK(phy, 10, 11, 24))
> +#define ICL_DPCLKA_CFGCR0_TC_CLK_OFF(tc_port) (1 << ((tc_port) == PORT_TC4 ? \
> + 21 : (tc_port) + 12))
> +#define ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy) ((phy) * 2)
> +#define ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy) (3 << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> +#define ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll, phy) ((pll) << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> +
> /* CNL PLL */
> #define DPLL0_ENABLE 0x46010
> #define DPLL1_ENABLE 0x46014
> --
> 2.17.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
