> -----Original Message-----
> From: Sousa, Gustavo <gustavo.so...@intel.com>
> Sent: Tuesday, September 12, 2023 6:59 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Kahola, Mika <mika.kah...@intel.com>; Taylor, Clinton A
> <clinton.a.tay...@intel.com>
> Subject: [PATCH v2] drm/i915/cx0: Add step for programming msgbus timer
>
> There was a recent update in the BSpec adding an extra step to the PLL enable
> sequence, which is for programming the msgbus
> timer. Since we also touch PHY registers during hw readout, let's do the
> programming when starting a transaction rather than only
> when doing the PLL enable sequence.
>
> This might be the missing step that was causing the timeouts that we have
> recently seen during C20 SRAM register programming
> sequences. With this in place, we shouldn't need the logic to bump the timer
> thresholds, since now we have a documented value
> that should be set peform programming the registers. As such, let's also
> remove intel_cx0_bus_check_and_bump_timer(), but
> keep the part that checks if hardware really detected a timeout, which might
> be useful debugging information.
>
> v2:
> - Use debug level instead of warning for the message notifying that
> the hardware did not detect the timeout. (Mika)
> - Got a new BSpec update clarifying that we need to program the msgbus
> timer of both PHY lanes. Update the changes to reflect that.
> (Gustavo)
>
> BSpec: 64568
> Cc: Mika Kahola <mika.kah...@intel.com>
Reviewed-by: Mika Kahola <mika.kah...@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.so...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 87 +++++++++----------
> .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 2 +-
> 2 files changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index e6d3027c821d..abd607b564f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -29,8 +29,6 @@
> #define INTEL_CX0_LANE1 BIT(1)
> #define INTEL_CX0_BOTH_LANES (INTEL_CX0_LANE1 | INTEL_CX0_LANE0)
>
> -#define INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL 0x200
> -
> bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy) {
> if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && phy < PHY_C) @@ -73,19
> +71,38 @@ assert_dc_off(struct
> drm_i915_private *i915)
> drm_WARN_ON(&i915->drm, !enabled);
> }
>
> +static void intel_cx0_program_msgbus_timer(struct intel_encoder
> +*encoder) {
> + int lane;
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +
> + for_each_cx0_lane_in_mask(INTEL_CX0_BOTH_LANES, lane)
> + intel_de_rmw(i915,
> + XELPDP_PORT_MSGBUS_TIMER(encoder->port, lane),
> + XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
> + XELPDP_PORT_MSGBUS_TIMER_VAL);
> +}
> +
> /*
> * Prepare HW for CX0 phy transactions.
> *
> * It is required that PSR and DC5/6 are disabled before any CX0 message
> * bus transaction is executed.
> + *
> + * We also do the msgbus timer programming here to ensure that the
> + timer
> + * is already programmed before any access to the msgbus.
> */
> static intel_wakeref_t intel_cx0_phy_transaction_begin(struct intel_encoder
> *encoder) {
> + intel_wakeref_t wakeref;
> struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>
> intel_psr_pause(intel_dp);
> - return intel_display_power_get(i915, POWER_DOMAIN_DC_OFF);
> + wakeref = intel_display_power_get(i915, POWER_DOMAIN_DC_OFF);
> + intel_cx0_program_msgbus_timer(encoder);
> +
> + return wakeref;
> }
>
> static void intel_cx0_phy_transaction_end(struct intel_encoder *encoder,
> intel_wakeref_t wakeref) @@ -121,42 +138,6 @@
> static void intel_cx0_bus_reset(struct drm_i915_private *i915, enum port
> port, i
> intel_clear_response_ready_flag(i915, port, lane); }
>
> -/*
> - * Check if there was a timeout detected by the hardware in the message bus
> - * and bump the threshold if so.
> - */
> -static void intel_cx0_bus_check_and_bump_timer(struct drm_i915_private *i915,
> - enum port port, int lane)
> -{
> - enum phy phy = intel_port_to_phy(i915, port);
> - i915_reg_t reg;
> - u32 val;
> - u32 timer_val;
> -
> - reg = XELPDP_PORT_MSGBUS_TIMER(port, lane);
> - val = intel_de_read(i915, reg);
> -
> - if (!(val & XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT)) {
> - drm_warn(&i915->drm,
> - "PHY %c lane %d: hardware did not detect a timeout\n",
> - phy_name(phy), lane);
> - return;
> - }
> -
> - timer_val = REG_FIELD_GET(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK, val);
> -
> - if (timer_val == INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL)
> - return;
> -
> - val &= ~XELPDP_PORT_MSGBUS_TIMER_VAL_MASK;
> - val |= XELPDP_PORT_MSGBUS_TIMER_VAL(INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL);
> -
> - drm_dbg_kms(&i915->drm,
> - "PHY %c lane %d: increasing msgbus timer threshold to
> %#x\n",
> - phy_name(phy), lane, INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL);
> - intel_de_write(i915, reg, val);
> -}
> -
> static int intel_cx0_wait_for_ack(struct drm_i915_private *i915, enum port
> port,
> int command, int lane, u32 *val)
> {
> @@ -170,7 +151,13 @@ static int intel_cx0_wait_for_ack(struct
> drm_i915_private *i915, enum port port,
> XELPDP_MSGBUS_TIMEOUT_SLOW, val)) {
> drm_dbg_kms(&i915->drm, "PHY %c Timeout waiting for message
> ACK. Status: 0x%x\n",
> phy_name(phy), *val);
> - intel_cx0_bus_check_and_bump_timer(i915, port, lane);
> +
> + if (!(intel_de_read(i915, XELPDP_PORT_MSGBUS_TIMER(port, lane))
> &
> + XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT))
> + drm_dbg_kms(&i915->drm,
> + "PHY %c Hardware did not detect a
> timeout\n",
> + phy_name(phy));
> +
> intel_cx0_bus_reset(i915, port, lane);
> return -ETIMEDOUT;
> }
> @@ -2737,39 +2724,45 @@ static void intel_cx0pll_enable(struct intel_encoder
> *encoder,
> intel_cx0_powerdown_change_sequence(i915, encoder->port,
> INTEL_CX0_BOTH_LANES,
> CX0_P2_STATE_READY);
>
> - /* 4. Program PHY internal PLL internal registers. */
> + /*
> + * 4. Program PORT_MSGBUS_TIMER register's Message Bus Timer field to
> 0xA000.
> + * (This is done inside intel_cx0_phy_transaction_begin(), since we
> would need
> + * the right timer thresholds for readouts too.)
> + */
> +
> + /* 5. Program PHY internal PLL internal registers. */
> if (intel_is_c10phy(i915, phy))
> intel_c10_pll_program(i915, crtc_state, encoder);
> else
> intel_c20_pll_program(i915, crtc_state, encoder);
>
> /*
> - * 5. Program the enabled and disabled owned PHY lane
> + * 6. Program the enabled and disabled owned PHY lane
> * transmitters over message bus
> */
> intel_cx0_program_phy_lane(i915, encoder, crtc_state->lane_count,
> lane_reversal);
>
> /*
> - * 6. Follow the Display Voltage Frequency Switching - Sequence
> + * 7. Follow the Display Voltage Frequency Switching - Sequence
> * Before Frequency Change. We handle this step in bxt_set_cdclk().
> */
>
> /*
> - * 7. Program DDI_CLK_VALFREQ to match intended DDI
> + * 8. Program DDI_CLK_VALFREQ to match intended DDI
> * clock frequency.
> */
> intel_de_write(i915, DDI_CLK_VALFREQ(encoder->port),
> crtc_state->port_clock);
>
> /*
> - * 8. Set PORT_CLOCK_CTL register PCLK PLL Request
> + * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> * LN<Lane for maxPCLK> to "1" to enable PLL.
> */
> intel_de_rmw(i915, XELPDP_PORT_CLOCK_CTL(encoder->port),
> intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> intel_cx0_get_pclk_pll_request(maxpclk_lane));
>
> - /* 9. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1".
> */
> + /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> ==
> +"1". */
> if (__intel_de_wait_for_register(i915,
> XELPDP_PORT_CLOCK_CTL(encoder->port),
>
> intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>
> intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> @@ -2778,7 +2771,7 @@ static void intel_cx0pll_enable(struct intel_encoder
> *encoder,
> phy_name(phy), XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US);
>
> /*
> - * 10. Follow the Display Voltage Frequency Switching Sequence After
> + * 11. Follow the Display Voltage Frequency Switching Sequence After
> * Frequency Change. We handle this step in bxt_set_cdclk().
> */
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> index b2db4cc366d6..adf8f4ce0d49 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -121,7 +121,7 @@
>
> _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC2) + (lane) * 4)
> #define XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT REG_BIT(31)
> #define XELPDP_PORT_MSGBUS_TIMER_VAL_MASK REG_GENMASK(23, 0)
> -#define XELPDP_PORT_MSGBUS_TIMER_VAL(val)
> REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
> val)
> +#define XELPDP_PORT_MSGBUS_TIMER_VAL
> REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
> 0xa000)
>
> #define _XELPDP_PORT_CLOCK_CTL_A 0x640E0
> #define _XELPDP_PORT_CLOCK_CTL_B 0x641E0
> --
> 2.42.0
