Quoting Jani Nikula (2023-08-29 12:12:19-03:00)
>On Mon, 28 Aug 2023, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>>>> +#define INTEL_CX0_MSGBUS_TIMER_VAL_MAX 0x200
>
>Either make this 0x200U (for unsigned)...
>
>>>> +
>>>> 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...
>
>...or use min_t() instead of adding the cast here.
The v2 of this patch removed the usage of min(), but thanks for the tips!
--
Gustavo Sousa
>
>BR,
>Jani.
>
>
>>
>> ...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.
>>
>> --
>> 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
>>>
>
>--
>Jani Nikula, Intel Open Source Graphics Center
