On Wed, 2019-07-03 at 18:06 -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.
>
Nit: Why not already rename DPCLKA_CFGCR0_ICL to ICL_DPCLKA_CFGCR0? The
bits have the new name, so you are already touching in everyplace that
uses DPCLKA_CFGCR0_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)
Reviewed-by: José Roberto de Souza <jose.so...@intel.com>
>
> Bspec: 33148
> Cc: José Roberto de Souza <jose.so...@intel.com>
> Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
> ---
> 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 : \
> (port) + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
