> -----Original Message----- > From: Aaron Esau <[email protected]> > Sent: Saturday, 9 May 2026 19.24 > To: [email protected] > Cc: [email protected]; [email protected]; > [email protected]; Vivi, Rodrigo > <[email protected]>; [email protected]; > [email protected]; Kahola, Mika <[email protected]>; > [email protected]; Aaron Esau <[email protected]> > Subject: [PATCH 1/3] drm/i915/cx0: check PLL ACK bit in > intel_cx0_pll_is_enabled() > > intel_cx0_pll_is_enabled() only checks the PCLK_PLL_REQUEST bit in > PORT_CLOCK_CTL, which is set by the driver during the PLL > enable sequence. It does not check the PCLK_PLL_ACK bit, which is the > hardware's response indicating the PLL actually locked. > > When the CX0 PHY MSGBUS is unresponsive (e.g. after a failed s2idle resume), > the PLL register programming via MSGBUS silently > fails and the PLL never locks, but intel_cx0_pll_is_enabled() returns true > because the driver-set REQUEST bit is present. This > causes all downstream state readout and verification to operate on a PLL that > is not actually enabled. > > Check both the REQUEST and ACK bits so that a PLL is only reported as enabled > when the hardware has confirmed it locked. > > Fixes: bf8531990380 ("drm/i915/display: Allow display PHYs to reset power > state") > Cc: [email protected] > Cc: Mika Kahola <[email protected]> > Signed-off-by: Aaron Esau <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > index 7288065d2..4cacea802 100644 > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > @@ -3581,9 +3581,12 @@ static bool intel_cx0_pll_is_enabled(struct > intel_encoder *encoder)
intel_cx0_pll_is_enabled() does not tell us that the PLL is enabled in HW, only that SW has set the enable/request bit. I agree the function name could be clearer about that since it really checks whether SW has initiated PLL enable. Given that, I don't think intel_cx0pll_readout_hw_state() or intel_cx0_pll_power_save_wa() needs to wait for the HW ack before reading state back. For this readout path, checking that enable was requested should be sufficient. -Mika- > struct intel_display *display = to_intel_display(encoder); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > u8 lane = dig_port->lane_reversal ? INTEL_CX0_LANE1 : INTEL_CX0_LANE0; > + u32 val; > > - return intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, > encoder->port)) & > - intel_cx0_get_pclk_pll_request(lane); > + val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, > +encoder->port)); > + > + return (val & intel_cx0_get_pclk_pll_request(lane)) && > + (val & intel_cx0_get_pclk_pll_ack(lane)); > } > > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder) > -- > 2.54.0
