Hi Gustavo,
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Gustavo
> Sousa
> Sent: Monday, August 28, 2023 10:34 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH] drm/i915/cx0: Check and increase msgbus timeout
> threshold
>
> We have experienced timeout issues when accessing C20 SRAM registers.
This wording is misleading. It seems to imply that we directly access SRAM
registers via msg bus but the reads/writes go through intermediate registers
and it is this read/write that is failing. Adding extra clarity here would be helpful.
> Experimentation showed that bumping the message bus timer threshold
> helped on getting display Type-C connection on the C20 PHY to work.
>
> While the timeout is still under investigation with the HW team, having
> logic to allow forward progress (with the proper warnings) seems useful.
> Thus, let's bump the threshold when a timeout is detected.
>
> The maximum value of 0x200 pclk cycles was somewhat arbitrary - 2x the
> default value. That value was successfully tested on real hardware that
> was displaying timeouts otherwise.
>
> BSpec: 65156
> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 41 +++++++++++++++++++
> .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 12 ++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index dd489b50ad60..55d2333032e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -5,6 +5,7 @@
>
> #include <linux/log2.h>
> #include <linux/math64.h>
> +#include <linux/minmax.h>
> #include "i915_reg.h"
> #include "intel_cx0_phy.h"
> #include "intel_cx0_phy_regs.h"
> @@ -29,6 +30,8 @@
> #define INTEL_CX0_LANE1 BIT(1)
> #define INTEL_CX0_BOTH_LANES (INTEL_CX0_LANE1 |
> INTEL_CX0_LANE0)
>
> +#define INTEL_CX0_MSGBUS_TIMER_VAL_MAX 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)
> @@ -119,6 +122,43 @@ 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_VAL_MAX)
> + return;
> +
> + timer_val = min(2 * timer_val,
> (u32)INTEL_CX0_MSGBUS_TIMER_VAL_MAX);
^ is this cast necessary?
> + val &= ~XELPDP_PORT_MSGBUS_TIMER_VAL_MASK;
> + val |= REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
> timer_val);
We do not use REG_FIELD_PREP in the function. Instead we use it in
register definition in intel_cx0_phy_regs.h.
Thanks,
Radhakrishna Sripada
> +
> + drm_dbg_kms(&i915->drm,
> + "PHY %c lane %d: increasing msgbus timer threshold to
> %#x\n",
> + phy_name(phy), lane, timer_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)
> {
> @@ -132,6 +172,7 @@ 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);
> intel_cx0_bus_reset(i915, port, lane);
> return -ETIMEDOUT;
> }
> 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 cb5d1be2ba19..17cc3385aabb 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -110,6 +110,18 @@
> #define CX0_P4PG_STATE_DISABLE 0xC
> #define CX0_P2_STATE_RESET 0x2
>
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_A
> 0x640d8
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_B
> 0x641d8
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC1 0x16f258
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC2 0x16f458
> +#define XELPDP_PORT_MSGBUS_TIMER(port, lane)
> _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \
> +
> _XELPDP_PORT_MSGBUS_TIMER_LN0_A, \
> +
> _XELPDP_PORT_MSGBUS_TIMER_LN0_B, \
> +
> _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC1, \
> +
> _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_CLOCK_CTL_A 0x640E0
> #define _XELPDP_PORT_CLOCK_CTL_B 0x641E0
> #define _XELPDP_PORT_CLOCK_CTL_USBC1 0x16F260
> --
> 2.41.0
