Hi Gustavo,
> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>
> Sent: Monday, August 28, 2023 2:46 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] drm/i915/cx0: Check and increase msgbus
> timeout threshold
>
> Quoting Sripada, Radhakrishna (2023-08-28 17:30:19-03:00)
> >Hi Gustavo,
>
> Hi, RK.
>
> Thanks for the feedback! Please, see my replies below.
>
> >
> >> -----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.
>
> Hm... Okay. I thought that would already be implied to someone familiar with the
> code. What about:
>
> s/when accessing/when going through the sequence to access/
This is better to indicate that it is not direct access.
>
> ?
>
> >
> >> 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?
>
> Yes. I remember getting a warning without it. Let me remove it to remember...
Got it thanks for the quick check.
>
> ...done! I think it complains because the arguments are of different types:
>
> In file included from drivers/gpu/drm/i915/display/intel_cx0_phy.c:8:
> drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function
> ‘intel_cx0_bus_check_and_bump_timer’:
> ./include/linux/minmax.h:20:35: error: comparison of distinct pointer types
> lacks a cast [-Werror]
> 20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> | ^~
> ./include/linux/minmax.h:26:18: note: in expansion of macro ‘__typecheck’
> 26 | (__typecheck(x, y) && __no_side_effects(x, y))
> | ^~~~~~~~~~~
> ./include/linux/minmax.h:36:31: note: in expansion of macro ‘__safe_cmp’
> 36 | __builtin_choose_expr(__safe_cmp(x, y), \
> | ^~~~~~~~~~
> ./include/linux/minmax.h:67:25: note: in expansion of macro ‘__careful_cmp’
> 67 | #define min(x, y) __careful_cmp(x, y, <)
> | ^~~~~~~~~~~~~
> drivers/gpu/drm/i915/display/intel_cx0_phy.c:152:21: note: in expansion of
> macro ‘min’
> 152 | timer_val = min(2 * timer_val,
> INTEL_CX0_MSGBUS_TIMER_VAL_MAX);
> |
>
> >
> >> + 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.
>
> I think it makes sense have definitions using REG_FIELD_PREP() in header files
> and use those definitions in .c files for fields whose values are are
> enumerated.
>
> Now, for fields without enumeration, like this one in discussion, I think it is
> fine to use REG_FIELD_PREP() directly. The MASK is already exposed anyway...
>
> Besides, it seems we already have lines of code in *.c files using the pattern
> above:
>
> git grep REG_FIELD_PREP drm-tip/drm-tip -- ':/drivers/gpu/drm/i915/**/*.c'
>
> Let me know what you think. I could add a XELPDP_PORT_MSGBUS_TIMER_VAL()
> macro
> if you think it is necessary. My personal take is that using REG_FIELD_PREP()
> for this case is fine.
Let us conform with the norms for the macro-fields used in this file instead of starting
a new pattern.
--Radhakrishna(RK) Sripada
>
> --
> Gustavo Sousa
>
> >
> >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
> >
