[Intel-gfx] [PATCH 0/3] AUX power well fixes

[Intel-gfx] [PATCH 0/3] AUX power well fixes

[Matt Roper]

A few fixes related to AUX power wells for various platforms:
 * EHL shouldn't use TC power well handling on C/D since all of its
   outputs are combo.
 * TGL shouldn't be applying Display WA #1178; doing so causes failures
   to enable power wells in some cases.

There's some ambiguity as to whether WA#1178 is even still relevant to
ICL or not; we're checking with the hardware architects on that, but
this series leaves it in place for ICL for now.

Matt Roper (3):
  drm/i915/ehl: Define EHL powerwells independently of ICL
  drm/i915/tgl: Drop Wa#1178
  drm/i915/icl: Cleanup combo PHY aux power well handlers

 .../drm/i915/display/intel_display_power.c    | 193 +++++++++++++++---
 1 file changed, 166 insertions(+), 27 deletions(-)

-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/3] drm/i915/ehl: Define EHL powerwells independently of ICL

[Matt Roper]

Outputs C and D on EHL are combo PHY outputs and thus should not be
using the same TC AUX power well handlers as ICL.  And even though
icl_combo_phy_aux_power_well_ops works okay for EHL/JSL combo PHYs none
of its special handling is actually necessary for this platform:
 * EHL/JSL don't actually need to program PORT_CL_DW12
 * Display WA #1178 does not apply to EHL/JSL

Thus we can simply drop back to using our standard "hsw-style" power
well ops for EHL AUX power wells.

Bspec: 4301
Fixes: f722b8c1e2a2 ("drm/i915/ehl: All EHL ports are combo phys")
Cc: Jose Souza <jose.souza@intel.com>
Cc: Bob Paauwe <bob.j.paauwe@intel.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 147 ++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 0b3dd2a3b94d..cf34427cc840 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -3688,6 +3688,151 @@ static const struct i915_power_well_desc icl_power_wells[] = {
 	},
 };
 
+static const struct i915_power_well_desc ehl_power_wells[] = {
+	{
+		.name = "always-on",
+		.always_on = true,
+		.domains = POWER_DOMAIN_MASK,
+		.ops = &i9xx_always_on_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+	},
+	{
+		.name = "power well 1",
+		/* Handled by the DMC firmware */
+		.always_on = true,
+		.domains = 0,
+		.ops = &hsw_power_well_ops,
+		.id = SKL_DISP_PW_1,
+		{
+			.hsw.regs = &hsw_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_PW_1,
+			.hsw.has_fuses = true,
+		},
+	},
+	{
+		.name = "DC off",
+		.domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS,
+		.ops = &gen9_dc_off_power_well_ops,
+		.id = SKL_DISP_DC_OFF,
+	},
+	{
+		.name = "power well 2",
+		.domains = ICL_PW_2_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = SKL_DISP_PW_2,
+		{
+			.hsw.regs = &hsw_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_PW_2,
+			.hsw.has_fuses = true,
+		},
+	},
+	{
+		.name = "power well 3",
+		.domains = ICL_PW_3_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &hsw_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_PW_3,
+			.hsw.irq_pipe_mask = BIT(PIPE_B),
+			.hsw.has_vga = true,
+			.hsw.has_fuses = true,
+		},
+	},
+	{
+		.name = "DDI A IO",
+		.domains = ICL_DDI_IO_A_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_ddi_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_DDI_A,
+		},
+	},
+	{
+		.name = "DDI B IO",
+		.domains = ICL_DDI_IO_B_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_ddi_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_DDI_B,
+		},
+	},
+	{
+		.name = "DDI C IO",
+		.domains = ICL_DDI_IO_C_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_ddi_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_DDI_C,
+		},
+	},
+	{
+		.name = "DDI D IO",
+		.domains = ICL_DDI_IO_D_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_ddi_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_DDI_D,
+		},
+	},
+	{
+		.name = "AUX A",
+		.domains = ICL_AUX_A_IO_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_aux_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_AUX_A,
+		},
+	},
+	{
+		.name = "AUX B",
+		.domains = ICL_AUX_B_IO_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_aux_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_AUX_B,
+		},
+	},
+	{
+		.name = "AUX C",
+		.domains = ICL_AUX_C_TC1_IO_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_aux_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_AUX_C,
+		},
+	},
+	{
+		.name = "AUX D",
+		.domains = ICL_AUX_D_TC2_IO_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &icl_aux_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_AUX_D,
+		},
+	},
+	{
+		.name = "power well 4",
+		.domains = ICL_PW_4_POWER_DOMAINS,
+		.ops = &hsw_power_well_ops,
+		.id = DISP_PW_ID_NONE,
+		{
+			.hsw.regs = &hsw_power_well_regs,
+			.hsw.idx = ICL_PW_CTL_IDX_PW_4,
+			.hsw.has_fuses = true,
+			.hsw.irq_pipe_mask = BIT(PIPE_C),
+		},
+	},
+};
+
 static const struct i915_power_well_desc tgl_power_wells[] = {
 	{
 		.name = "always-on",
@@ -4162,6 +4307,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 	 */
 	if (IS_GEN(dev_priv, 12)) {
 		err = set_power_wells(power_domains, tgl_power_wells);
+	} else if (IS_ELKHARTLAKE(dev_priv)) {
+		err = set_power_wells(power_domains, ehl_power_wells);
 	} else if (IS_GEN(dev_priv, 11)) {
 		err = set_power_wells(power_domains, icl_power_wells);
 	} else if (IS_CANNONLAKE(dev_priv)) {
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/3] drm/i915/tgl: Drop Wa#1178

[Matt Roper]

The TGL workaround database no longer shows Wa #1178 (or anything
similar under different workaround names/numbers) so we should be able
to drop it.  In fact Swati just discovered that applying this workaround
is the root cause of some power well enable failures we've been seeing
in CI (gitlab issue 498).

Once we stop applying this WA, TGL no longer utilizes any of the special
handling provided by icl_combo_phy_aux_power_well_ops so we can just
drop back to using the standard hsw-style power well ops instead.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/498
Fixes: deea06b47574 ("drm/i915/tgl: apply Display WA #1178 to fix type C dongles")
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Swati Sharma <swati2.sharma@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_power.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index cf34427cc840..52f2332e0ab8 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -3977,7 +3977,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
 	{
 		.name = "AUX A",
 		.domains = TGL_AUX_A_IO_POWER_DOMAINS,
-		.ops = &icl_combo_phy_aux_power_well_ops,
+		.ops = &hsw_power_well_ops,
 		.id = DISP_PW_ID_NONE,
 		{
 			.hsw.regs = &icl_aux_power_well_regs,
@@ -3987,7 +3987,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
 	{
 		.name = "AUX B",
 		.domains = TGL_AUX_B_IO_POWER_DOMAINS,
-		.ops = &icl_combo_phy_aux_power_well_ops,
+		.ops = &hsw_power_well_ops,
 		.id = DISP_PW_ID_NONE,
 		{
 			.hsw.regs = &icl_aux_power_well_regs,
@@ -3997,7 +3997,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
 	{
 		.name = "AUX C",
 		.domains = TGL_AUX_C_IO_POWER_DOMAINS,
-		.ops = &icl_combo_phy_aux_power_well_ops,
+		.ops = &hsw_power_well_ops,
 		.id = DISP_PW_ID_NONE,
 		{
 			.hsw.regs = &icl_aux_power_well_regs,
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915/tgl: Drop Wa#1178

[Lucas De Marchi]

On Thu, Dec 12, 2019 at 02:37:34PM -0800, Matt Roper wrote:
>The TGL workaround database no longer shows Wa #1178 (or anything
>similar under different workaround names/numbers) so we should be able
>to drop it.  In fact Swati just discovered that applying this workaround
>is the root cause of some power well enable failures we've been seeing
>in CI (gitlab issue 498).
>
>Once we stop applying this WA, TGL no longer utilizes any of the special
>handling provided by icl_combo_phy_aux_power_well_ops so we can just
>drop back to using the standard hsw-style power well ops instead.
>
>Closes: https://gitlab.freedesktop.org/drm/intel/issues/498
>Fixes: deea06b47574 ("drm/i915/tgl: apply Display WA #1178 to fix type C dongles")
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Cc: Swati Sharma <swati2.sharma@intel.com>
>Cc: Imre Deak <imre.deak@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>---
> drivers/gpu/drm/i915/display/intel_display_power.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>index cf34427cc840..52f2332e0ab8 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_power.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>@@ -3977,7 +3977,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> 	{
> 		.name = "AUX A",
> 		.domains = TGL_AUX_A_IO_POWER_DOMAINS,
>-		.ops = &icl_combo_phy_aux_power_well_ops,
>+		.ops = &hsw_power_well_ops,
> 		.id = DISP_PW_ID_NONE,
> 		{
> 			.hsw.regs = &icl_aux_power_well_regs,
>@@ -3987,7 +3987,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> 	{
> 		.name = "AUX B",
> 		.domains = TGL_AUX_B_IO_POWER_DOMAINS,
>-		.ops = &icl_combo_phy_aux_power_well_ops,
>+		.ops = &hsw_power_well_ops,
> 		.id = DISP_PW_ID_NONE,
> 		{
> 			.hsw.regs = &icl_aux_power_well_regs,
>@@ -3997,7 +3997,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> 	{
> 		.name = "AUX C",
> 		.domains = TGL_AUX_C_IO_POWER_DOMAINS,
>-		.ops = &icl_combo_phy_aux_power_well_ops,
>+		.ops = &hsw_power_well_ops,
> 		.id = DISP_PW_ID_NONE,
> 		{
> 			.hsw.regs = &icl_aux_power_well_regs,
>-- 
>2.23.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/3] drm/i915/icl: Cleanup combo PHY aux power well handlers

[Matt Roper]

Now that the combo PHY aux power well handlers are used exclusively on
Icelake, we can drop a bunch of the extra tests.  While we're at it,
also switch these over to using intel_uncore_rmw() for the relevant
register updates.

Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 40 ++++++++-----------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 52f2332e0ab8..f02667a4e62b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -418,31 +418,23 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 	int pw_idx = power_well->desc->hsw.idx;
 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
 	u32 val;
-	int wa_idx_max;
+
+	WARN_ON(!IS_ICELAKE(dev_priv));
+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
 
 	val = I915_READ(regs->driver);
 	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
 
-	if (INTEL_GEN(dev_priv) < 12) {
-		val = I915_READ(ICL_PORT_CL_DW12(phy));
-		I915_WRITE(ICL_PORT_CL_DW12(phy), val | ICL_LANE_ENABLE_AUX);
-	}
+	intel_uncore_rmw(&dev_priv->uncore, ICL_PORT_CL_DW12(phy), 0,
+			 ICL_LANE_ENABLE_AUX);
 
 	hsw_wait_for_power_well_enable(dev_priv, power_well);
 
-	/* Display WA #1178: icl, tgl */
-	if (IS_TIGERLAKE(dev_priv))
-		wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
-	else
-		wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
-
-	if (!IS_ELKHARTLAKE(dev_priv) &&
-	    pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
-	    !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
-		val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
-		val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
-		I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
-	}
+	if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
+	    !intel_bios_is_port_edp(dev_priv, (enum port)phy))
+		intel_uncore_rmw(&dev_priv->uncore, ICL_AUX_ANAOVRD1(pw_idx), 0,
+				 ICL_AUX_ANAOVRD1_ENABLE |
+				 ICL_AUX_ANAOVRD1_LDO_BYPASS);
 }
 
 static void
@@ -454,13 +446,13 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
 	u32 val;
 
-	if (INTEL_GEN(dev_priv) < 12) {
-		val = I915_READ(ICL_PORT_CL_DW12(phy));
-		I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
-	}
+	WARN_ON(!IS_ICELAKE(dev_priv));
+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
 
-	val = I915_READ(regs->driver);
-	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
+	intel_uncore_rmw(&dev_priv->uncore, ICL_PORT_CL_DW12(phy),
+			 ICL_LANE_ENABLE_AUX, 0);
+	intel_uncore_rmw(&dev_priv->uncore, regs->driver,
+			 HSW_PWR_WELL_CTL_REQ(pw_idx), 0);
 
 	hsw_wait_for_power_well_disable(dev_priv, power_well);
 }
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915/icl: Cleanup combo PHY aux power well handlers

[Lucas De Marchi]

On Thu, Dec 12, 2019 at 02:37:35PM -0800, Matt Roper wrote:
>Now that the combo PHY aux power well handlers are used exclusively on
>Icelake, we can drop a bunch of the extra tests.  While we're at it,
>also switch these over to using intel_uncore_rmw() for the relevant
>register updates.

display/ shouldn't use intel_uncore_* yet as Jani has a series to
introduce intel_de_ (intel_display_ ?)

Lucas De Marchi

>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> .../drm/i915/display/intel_display_power.c    | 40 ++++++++-----------
> 1 file changed, 16 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>index 52f2332e0ab8..f02667a4e62b 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_power.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>@@ -418,31 +418,23 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> 	int pw_idx = power_well->desc->hsw.idx;
> 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> 	u32 val;
>-	int wa_idx_max;
>+
>+	WARN_ON(!IS_ICELAKE(dev_priv));
>+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
>
> 	val = I915_READ(regs->driver);
> 	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
>
>-	if (INTEL_GEN(dev_priv) < 12) {
>-		val = I915_READ(ICL_PORT_CL_DW12(phy));
>-		I915_WRITE(ICL_PORT_CL_DW12(phy), val | ICL_LANE_ENABLE_AUX);
>-	}
>+	intel_uncore_rmw(&dev_priv->uncore, ICL_PORT_CL_DW12(phy), 0,
>+			 ICL_LANE_ENABLE_AUX);
>
> 	hsw_wait_for_power_well_enable(dev_priv, power_well);
>
>-	/* Display WA #1178: icl, tgl */
>-	if (IS_TIGERLAKE(dev_priv))
>-		wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
>-	else
>-		wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
>-
>-	if (!IS_ELKHARTLAKE(dev_priv) &&
>-	    pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
>-	    !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
>-		val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
>-		val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
>-		I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
>-	}
>+	if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
>+	    !intel_bios_is_port_edp(dev_priv, (enum port)phy))
>+		intel_uncore_rmw(&dev_priv->uncore, ICL_AUX_ANAOVRD1(pw_idx), 0,
>+				 ICL_AUX_ANAOVRD1_ENABLE |
>+				 ICL_AUX_ANAOVRD1_LDO_BYPASS);
> }
>
> static void
>@@ -454,13 +446,13 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> 	u32 val;
>
>-	if (INTEL_GEN(dev_priv) < 12) {
>-		val = I915_READ(ICL_PORT_CL_DW12(phy));
>-		I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
>-	}
>+	WARN_ON(!IS_ICELAKE(dev_priv));
>+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
>
>-	val = I915_READ(regs->driver);
>-	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
>+	intel_uncore_rmw(&dev_priv->uncore, ICL_PORT_CL_DW12(phy),
>+			 ICL_LANE_ENABLE_AUX, 0);
>+	intel_uncore_rmw(&dev_priv->uncore, regs->driver,
>+			 HSW_PWR_WELL_CTL_REQ(pw_idx), 0);
>
> 	hsw_wait_for_power_well_disable(dev_priv, power_well);
> }
>-- 
>2.23.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/3] drm/i915/icl: Cleanup combo PHY aux power well handlers

[Matt Roper]

Now that the combo PHY aux power well handlers are used exclusively on
Icelake, we can drop a bunch of the extra tests.

v2: Don't try to use intel_uncore_rmw for register updates yet; there's
    pending display uncore patches that need to land first.  (Lucas)

Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 25 ++++++++-----------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 52f2332e0ab8..a0669dc15540 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -418,7 +418,9 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 	int pw_idx = power_well->desc->hsw.idx;
 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
 	u32 val;
-	int wa_idx_max;
+
+	WARN_ON(!IS_ICELAKE(dev_priv));
+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
 
 	val = I915_READ(regs->driver);
 	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
@@ -430,19 +432,11 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 
 	hsw_wait_for_power_well_enable(dev_priv, power_well);
 
-	/* Display WA #1178: icl, tgl */
-	if (IS_TIGERLAKE(dev_priv))
-		wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
-	else
-		wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
-
-	if (!IS_ELKHARTLAKE(dev_priv) &&
-	    pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
-	    !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
+	if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
+	    !intel_bios_is_port_edp(dev_priv, (enum port)phy))
 		val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
 		val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
 		I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
-	}
 }
 
 static void
@@ -454,10 +448,11 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
 	u32 val;
 
-	if (INTEL_GEN(dev_priv) < 12) {
-		val = I915_READ(ICL_PORT_CL_DW12(phy));
-		I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
-	}
+	WARN_ON(!IS_ICELAKE(dev_priv));
+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
+
+	val = I915_READ(ICL_PORT_CL_DW12(phy));
+	I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
 
 	val = I915_READ(regs->driver);
 	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/icl: Cleanup combo PHY aux power well handlers

[Lucas De Marchi]

On Thu, Dec 12, 2019 at 03:51:21PM -0800, Matt Roper wrote:
>Now that the combo PHY aux power well handlers are used exclusively on
>Icelake, we can drop a bunch of the extra tests.
>
>v2: Don't try to use intel_uncore_rmw for register updates yet; there's
>    pending display uncore patches that need to land first.  (Lucas)
>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> .../drm/i915/display/intel_display_power.c    | 25 ++++++++-----------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>index 52f2332e0ab8..a0669dc15540 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_power.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>@@ -418,7 +418,9 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> 	int pw_idx = power_well->desc->hsw.idx;
> 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> 	u32 val;
>-	int wa_idx_max;
>+
>+	WARN_ON(!IS_ICELAKE(dev_priv));
>+	WARN_ON(intel_phy_is_combo(dev_priv, phy));

did you mean !intel_phy_is_combo()?

I'm not sure I actually like the warns, we could just live without them.

We should also remove  _TGL_AUX_ANAOVRD1_C either in this commit or in
the previous one.

Lucas De Marchi

>
> 	val = I915_READ(regs->driver);
> 	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
>@@ -430,19 +432,11 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
>
> 	hsw_wait_for_power_well_enable(dev_priv, power_well);
>
>-	/* Display WA #1178: icl, tgl */
>-	if (IS_TIGERLAKE(dev_priv))
>-		wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
>-	else
>-		wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
>-
>-	if (!IS_ELKHARTLAKE(dev_priv) &&
>-	    pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
>-	    !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
>+	if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
>+	    !intel_bios_is_port_edp(dev_priv, (enum port)phy))
> 		val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
> 		val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
> 		I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
>-	}
> }
>
> static void
>@@ -454,10 +448,11 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> 	u32 val;
>
>-	if (INTEL_GEN(dev_priv) < 12) {
>-		val = I915_READ(ICL_PORT_CL_DW12(phy));
>-		I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
>-	}
>+	WARN_ON(!IS_ICELAKE(dev_priv));
>+	WARN_ON(intel_phy_is_combo(dev_priv, phy));
>+
>+	val = I915_READ(ICL_PORT_CL_DW12(phy));
>+	I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
>
> 	val = I915_READ(regs->driver);
> 	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
>-- 
>2.23.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/icl: Cleanup combo PHY aux power well handlers

[Matt Roper]

On Thu, Dec 12, 2019 at 03:56:38PM -0800, Lucas De Marchi wrote:
> On Thu, Dec 12, 2019 at 03:51:21PM -0800, Matt Roper wrote:
> > Now that the combo PHY aux power well handlers are used exclusively on
> > Icelake, we can drop a bunch of the extra tests.
> > 
> > v2: Don't try to use intel_uncore_rmw for register updates yet; there's
> >    pending display uncore patches that need to land first.  (Lucas)
> > 
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > .../drm/i915/display/intel_display_power.c    | 25 ++++++++-----------
> > 1 file changed, 10 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 52f2332e0ab8..a0669dc15540 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -418,7 +418,9 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> > 	int pw_idx = power_well->desc->hsw.idx;
> > 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> > 	u32 val;
> > -	int wa_idx_max;
> > +
> > +	WARN_ON(!IS_ICELAKE(dev_priv));
> > +	WARN_ON(intel_phy_is_combo(dev_priv, phy));
> 
> did you mean !intel_phy_is_combo()?

Woops, yes!  I'll send a followup shortly.

> 
> I'm not sure I actually like the warns, we could just live without them.

I feel like power wells are an area where we tend to do a lot of
copy/paste/modify when defining a new platform because the tables are so
large and a lot of the entries are similar or stay the same.  I want to
make sure we don't cargo cult the ICL-specific function onto future
platforms by accident.  It's easy to miss those mistakes in code review
because it's hooking up a combo phy-specific table which *sounds* like
the right thing to do on the surface if you don't realize the relevant
functions have ICL-specific handling that we don't want to propagate
forward.

Granted, people are more likely to copy TGL than ICL at this point, but
I figured the extra assertions wouldn't hurt.


> 
> We should also remove  _TGL_AUX_ANAOVRD1_C either in this commit or in
> the previous one.

Yeah, good point.  I'll do that too.


Matt

> 
> Lucas De Marchi
> 
> > 
> > 	val = I915_READ(regs->driver);
> > 	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> > @@ -430,19 +432,11 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> > 
> > 	hsw_wait_for_power_well_enable(dev_priv, power_well);
> > 
> > -	/* Display WA #1178: icl, tgl */
> > -	if (IS_TIGERLAKE(dev_priv))
> > -		wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
> > -	else
> > -		wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
> > -
> > -	if (!IS_ELKHARTLAKE(dev_priv) &&
> > -	    pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
> > -	    !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
> > +	if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
> > +	    !intel_bios_is_port_edp(dev_priv, (enum port)phy))
> > 		val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
> > 		val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
> > 		I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
> > -	}
> > }
> > 
> > static void
> > @@ -454,10 +448,11 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> > 	enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> > 	u32 val;
> > 
> > -	if (INTEL_GEN(dev_priv) < 12) {
> > -		val = I915_READ(ICL_PORT_CL_DW12(phy));
> > -		I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
> > -	}
> > +	WARN_ON(!IS_ICELAKE(dev_priv));
> > +	WARN_ON(intel_phy_is_combo(dev_priv, phy));
> > +
> > +	val = I915_READ(ICL_PORT_CL_DW12(phy));
> > +	I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
> > 
> > 	val = I915_READ(regs->driver);
> > 	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> > -- 
> > 2.23.0
> > 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx