Looks good, I only have some nitpicks.
On Wed, 2023-05-24 at 18:01 +0300, Mika Kahola wrote:
> In case when only two or less lanes are owned such as MFD (DP-alt with x2
> lanes)
> we need to reset only one lane (lane0). With only x2 lanes we don't need
> to poll for the phy current status on both lanes since only the owned lane
> will respond.
It would be nice to explain why it is so.
> Signed-off-by: Mika Kahola <mika.kah...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39 ++++++++++++--------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index ee6902118860..b8c812c5b33f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2528,13 +2528,23 @@ static u32 intel_cx0_get_pclk_refclk_ack(u8 lane_mask)
> return val;
> }
>
> -/* FIXME: Some Type-C cases need not reset both the lanes. Handle those
> cases. */
> -static void intel_cx0_phy_lane_reset(struct drm_i915_private *i915, enum
> port port,
> +static void intel_cx0_phy_lane_reset(struct drm_i915_private *i915,
> + struct intel_encoder *encoder,
> bool lane_reversal)
> {
> + enum port port = encoder->port;
> enum phy phy = intel_port_to_phy(i915, port);
> + u8 fia_max =
> intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder));
Logically, we don't care about "fia_max" in this function, we only care
whether one or two lanes should be handled. In all places we use
fia_max, we only check if it is > 2. So I think it would be clearer to
have the > 2 here already and call the variable something else.
Additionally, "> 2" looks slightly magic (without knowing the reason,
as stated in my first comment). Is there any more self-explanatory
symbol we could used?
> u8 lane_mask = lane_reversal ? INTEL_CX0_LANE1 :
> INTEL_CX0_LANE0;
> + u32 lane_pipe_reset = fia_max > 2 ?
> + XELPDP_LANE_PIPE_RESET(0) |
> + XELPDP_LANE_PIPE_RESET(1) :
> + XELPDP_LANE_PIPE_RESET(0);
> + u32 lane_phy_current_status = fia_max > 2 ?
> + XELPDP_LANE_PHY_CURRENT_STATUS(0) |
> + XELPDP_LANE_PHY_CURRENT_STATUS(1) :
> + XELPDP_LANE_PHY_CURRENT_STATUS(0);
It was already logically like this in the code, so not directly related
to this patch, but is there any reason why we don't need to include
more lanes in the reset? I mean, we're only handling lanes 0 and 1. If
we have 4 lanes, the other two never need to be reset?
--
Cheers,
Luca.
