From: "Souza, Jose" <jose.souza@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Use single set of AUX powerwell ops for gen11+
Date: Tue, 14 Apr 2020 22:28:00 +0000 [thread overview]
Message-ID: <7025ac2e3502ce9193d2aa9ee144b7891f732d0c.camel@intel.com> (raw)
In-Reply-To: <20200414205755.2777261-1-matthew.d.roper@intel.com>
On Tue, 2020-04-14 at 13:57 -0700, Matt Roper wrote:
> AUX power wells sometimes need additional handling besides just
> programming the specific power well registers:
> * Type-C PHY's also require additional Type-C register programming
> * ICL combo PHY's require additional workarounds
> * TGL & EHL combo PHY's can be treated like any other power well
>
> Today we have dedicated aux ops for the ICL combo PHY and Type-C
> cases.
> This works fine, but means that when a new platform shows up with
> identical general power well handling, but different types of PHYs on
> its outputs, we have to define an entire new power well table for
> that
> platform and can't just re-use the table from the earlier platform --
> as
> an example, see ehl_power_wells[], which is a subset of
> icl_power_wells[], *except* that we need to specify different AUX ops
> for the third display.
>
> If we instead create a single set of top-level aux ops that will
> check
> the PHY type and then dispatch to the appropriate handlers, we can
> get
> more reuse out of our power well definitions. This allows us to
> immediately eliminate ehl_power_wells[] and simply reuse the ICL
> table;
> if future platforms follow the same general power well assignments as
> either ICL or TGL, we'll be able to re-use those tables in the same
> way.
>
> Note that I've only changed ICL+ platforms over to using the new
> icl_aux
> ops; at this point it's unlikely that we'll have any new platforms
> that
> re-use gen9 or earlier power well configurations.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> .../drm/i915/display/intel_display_power.c | 242 +++++-----------
> --
> 1 file changed, 60 insertions(+), 182 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 433e5a81dd4d..ddeaa475b8a4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -593,6 +593,38 @@ icl_tc_phy_aux_power_well_disable(struct
> drm_i915_private *dev_priv,
> hsw_power_well_disable(dev_priv, power_well);
> }
>
> +static void
> +icl_aux_power_well_enable(struct drm_i915_private *dev_priv,
> + struct i915_power_well *power_well)
> +{
> + int pw_idx = power_well->desc->hsw.idx;
> + enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> +
The above will not work for TBT aux powerwells.
Other than that looks a good idea.
> + if (intel_phy_is_tc(dev_priv, phy))
> + return icl_tc_phy_aux_power_well_enable(dev_priv,
> power_well);
> + else if (IS_ICELAKE(dev_priv))
> + return icl_combo_phy_aux_power_well_enable(dev_priv,
> + power_well);
> + else
> + return hsw_power_well_enable(dev_priv, power_well);
> +}
> +
> +static void
> +icl_aux_power_well_disable(struct drm_i915_private *dev_priv,
> + struct i915_power_well *power_well)
> +{
> + int pw_idx = power_well->desc->hsw.idx;
> + enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> +
> + if (intel_phy_is_tc(dev_priv, phy))
> + return icl_tc_phy_aux_power_well_disable(dev_priv,
> power_well);
> + else if (IS_ICELAKE(dev_priv))
> + return icl_combo_phy_aux_power_well_disable(dev_priv,
> + power_well)
> ;
> + else
> + return hsw_power_well_disable(dev_priv, power_well);
> +}
> +
> /*
> * We should only use the power well if we explicitly asked the
> hardware to
> * enable it, so check if it's enabled and also check if we've
> requested it to
> @@ -3503,17 +3535,10 @@ static const struct i915_power_well_desc
> cnl_power_wells[] = {
> },
> };
>
> -static const struct i915_power_well_ops
> icl_combo_phy_aux_power_well_ops = {
> +static const struct i915_power_well_ops icl_aux_power_well_ops = {
> .sync_hw = hsw_power_well_sync_hw,
> - .enable = icl_combo_phy_aux_power_well_enable,
> - .disable = icl_combo_phy_aux_power_well_disable,
> - .is_enabled = hsw_power_well_enabled,
> -};
> -
> -static const struct i915_power_well_ops
> icl_tc_phy_aux_power_well_ops = {
> - .sync_hw = hsw_power_well_sync_hw,
> - .enable = icl_tc_phy_aux_power_well_enable,
> - .disable = icl_tc_phy_aux_power_well_disable,
> + .enable = icl_aux_power_well_enable,
> + .disable = icl_aux_power_well_disable,
> .is_enabled = hsw_power_well_enabled,
> };
>
> @@ -3643,7 +3668,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX A",
> .domains = ICL_AUX_A_IO_POWER_DOMAINS,
> - .ops = &icl_combo_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3653,7 +3678,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX B",
> .domains = ICL_AUX_B_IO_POWER_DOMAINS,
> - .ops = &icl_combo_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3663,7 +3688,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX C TC1",
> .domains = ICL_AUX_C_TC1_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3674,7 +3699,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX D TC2",
> .domains = ICL_AUX_D_TC2_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3685,7 +3710,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX E TC3",
> .domains = ICL_AUX_E_TC3_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3696,7 +3721,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX F TC4",
> .domains = ICL_AUX_F_TC4_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3707,7 +3732,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX C TBT1",
> .domains = ICL_AUX_C_TBT1_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3718,7 +3743,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX D TBT2",
> .domains = ICL_AUX_D_TBT2_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3729,7 +3754,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX E TBT3",
> .domains = ICL_AUX_E_TBT3_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3740,7 +3765,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> {
> .name = "AUX F TBT4",
> .domains = ICL_AUX_F_TBT4_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -3762,151 +3787,6 @@ 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",
> @@ -4051,7 +3931,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX A",
> .domains = TGL_AUX_A_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4061,7 +3941,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX B",
> .domains = TGL_AUX_B_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4071,7 +3951,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX C",
> .domains = TGL_AUX_C_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4081,7 +3961,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX D TC1",
> .domains = TGL_AUX_D_TC1_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4092,7 +3972,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX E TC2",
> .domains = TGL_AUX_E_TC2_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4103,7 +3983,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX F TC3",
> .domains = TGL_AUX_F_TC3_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4114,7 +3994,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX G TC4",
> .domains = TGL_AUX_G_TC4_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4125,7 +4005,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX H TC5",
> .domains = TGL_AUX_H_TC5_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4136,7 +4016,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX I TC6",
> .domains = TGL_AUX_I_TC6_IO_POWER_DOMAINS,
> - .ops = &icl_tc_phy_aux_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4147,7 +4027,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX D TBT1",
> .domains = TGL_AUX_D_TBT1_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4158,7 +4038,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX E TBT2",
> .domains = TGL_AUX_E_TBT2_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4169,7 +4049,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX F TBT3",
> .domains = TGL_AUX_F_TBT3_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4180,7 +4060,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX G TBT4",
> .domains = TGL_AUX_G_TBT4_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4191,7 +4071,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX H TBT5",
> .domains = TGL_AUX_H_TBT5_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4202,7 +4082,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> {
> .name = "AUX I TBT6",
> .domains = TGL_AUX_I_TBT6_IO_POWER_DOMAINS,
> - .ops = &hsw_power_well_ops,
> + .ops = &icl_aux_power_well_ops,
> .id = DISP_PW_ID_NONE,
> {
> .hsw.regs = &icl_aux_power_well_regs,
> @@ -4383,8 +4263,6 @@ 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)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-04-14 22:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-14 20:57 [Intel-gfx] [PATCH] drm/i915: Use single set of AUX powerwell ops for gen11+ Matt Roper
2020-04-14 22:28 ` Souza, Jose [this message]
2020-04-15 2:35 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for " Patchwork
2020-04-15 2:42 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-04-15 21:39 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7025ac2e3502ce9193d2aa9ee144b7891f732d0c.camel@intel.com \
--to=jose.souza@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.