> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Wednesday, 29 January 2025 16.45
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-
> xe@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>
> Subject: Re: [PATCH 2/2] drm/i915/display: Allow display PHYs to reset power
> state
>
> On Wed, 29 Jan 2025, Mika Kahola <mika.kahola@xxxxxxxxx> wrote:
> > The dedicated display PHYs reset to a power state that blocks S0ix,
> > increasing idle system power. After a system reset (cold boot, S3/4/5,
> > warm reset) if a dedicated PHY is not being brought up shortly, use
> > these steps to move the PHY to the lowest power state to save power.
> >
> > 1. Follow the PLL Enable Sequence, using any valid frequency such as DP 1.62
> GHz.
> > This brings lanes out of reset and enables the PLL to allow powerdown to be
> moved
> > to the Disable state.
> > 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state
> and disables the PLL.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 35
> > +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cx0_phy.h | 1 +
> > .../drm/i915/display/intel_display_reset.c | 2 ++
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 ++
> > 4 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index bffe3d4bbe8b..0bd74e899221 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -3559,3 +3559,38 @@ void intel_cx0pll_state_verify(struct
> intel_atomic_state *state,
> > else
> > intel_c20pll_state_verify(new_crtc_state, crtc, encoder,
> > &mpll_hw_state.c20); }
> > +
> > +/*
> > + * WA 14022081154
> > + * The dedicated display PHYs reset to a power state that blocks
> > +S0ix, increasing idle
> > + * system power. After a system reset (cold boot, S3/4/5, warm reset)
> > +if a dedicated
> > + * PHY is not being brought up shortly, use these steps to move the
> > +PHY to the lowest
> > + * power state to save power.
> > + *
> > + * 1. Follow the PLL Enable Sequence, using any valid frequency such as DP
> 1.62 GHz.
> > + * This brings lanes out of reset and enables the PLL to allow powerdown to
> be moved
> > + * to the Disable state.
> > + * 2. Follow PLL Disable Sequence. This moves powerdown to the Disable state
> and disables the PLL.
> > + */
> > +void wa_14022081154(struct intel_display *display)
>
> Please do not name functions like this. There's zero indication what this is about.
> You're adding a long comment what's going on, surely you can name the function
> accordingly?
Ok, I will change the name. I wasn't really sure how we should name these wa's. I've seen both ways to be used. It's true that this is not very descriptive name for a function.
>
> > +{
> > + struct intel_encoder *encoder;
> > + u32 val;
> > +
> > + if (DISPLAY_VER(display) < 30)
> > + return;
> > +
> > + for_each_intel_encoder(display->drm, encoder) {
> > + if (encoder->port == PORT_A || encoder->port == PORT_B) {
> > + val = intel_de_read(display,
> XELPDP_PORT_CLOCK_CTL(display,
> > +encoder->port));
> > +
> > + if (REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK,
> val) == XELPDP_DDI_CLOCK_SELECT_NONE) {
> > + struct intel_cx0pll_state pll_state = {};
> > + int port_clock = 162000;
> > + intel_c10pll_calc_state_from_table(encoder,
> mtl_c10_edp_tables, port_clock, true, &pll_state);
> > + __intel_cx0pll_enable(encoder, &pll_state, true,
> port_clock, 4);
> > + intel_cx0pll_disable(encoder);
> > + }
> > + }
> > + }
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > index 573fa7d3e88f..abebe7fd2cf2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > @@ -42,5 +42,6 @@ bool intel_cx0pll_compare_hw_state(const struct
> > intel_cx0pll_state *a, void intel_cx0_phy_set_signal_levels(struct
> intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state); int
> > intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder);
> > +void wa_14022081154(struct intel_display *display);
> >
> > #endif /* __INTEL_CX0_PHY_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c
> > b/drivers/gpu/drm/i915/display/intel_display_reset.c
> > index 093b386c95e8..93b2697a0876 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_reset.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c
> > @@ -7,6 +7,7 @@
> >
> > #include "i915_drv.h"
> > #include "intel_clock_gating.h"
> > +#include "intel_cx0_phy.h"
> > #include "intel_display_driver.h"
> > #include "intel_display_reset.h"
> > #include "intel_display_types.h"
> > @@ -116,6 +117,7 @@ void intel_display_reset_finish(struct drm_i915_private
> *i915)
> > intel_pps_unlock_regs_wa(display);
> > intel_display_driver_init_hw(display);
> > intel_clock_gating_init(i915);
> > + wa_14022081154(display);
>
> Contrast with intel_pps_unlock_regs_wa() call above.
Got it!
>
> > intel_hpd_init(i915);
> >
> > ret = __intel_display_driver_resume(display, state, ctx); diff
> > --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index b8fa04d3cd5c..76394411dd7a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -27,6 +27,7 @@
> > #include "bxt_dpio_phy_regs.h"
> > #include "i915_drv.h"
> > #include "i915_reg.h"
> > +#include "intel_cx0_phy.h"
> > #include "intel_de.h"
> > #include "intel_display_types.h"
> > #include "intel_dkl_phy.h"
> > @@ -4552,6 +4553,7 @@ static void sanitize_dpll_state(struct
> drm_i915_private *i915,
> > return;
> >
> > adlp_cmtg_clock_gating_wa(i915, pll);
> > + wa_14022081154(&i915->display);
>
> Contrast with adlp_cmtg_clock_gating_wa() above.
Got it!
>
> Imagine all of these were wa_32148191827(), wa_650914334(),
> wa_134174341(), etc. It's fine for compilers, not for humans.
I do agree. I will fix the naming.
Thanks for the review and comments!
-Mika-
>
> >
> > if (pll->active_mask)
> > return;
>
> --
> Jani Nikula, Intel
