[Intel-gfx] [PATCH 2/2] drm/i915: Explain the magic numbers for AUX SYNC/precharge length

[Hogander, Jouni]

On Thu, 2023-03-30 at 07:22 +0000, Hogander, Jouni wrote:
> On Wed, 2023-03-29 at 20:24 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Replace the hardcoded final numbers in the AUX SYNC/precharge
> > setup, and derive those from numbers from the (e)DP specs.
> > 
> > The new functions can serve as the single point of truth for
> > the number of SYNC pulses we use.
> > 
> > Cc: Jouni Högander <jouni.hogander at intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Let's take care of that io/fast wake calculation separately. I think we
want to have these in as currently fast wake SYNC pulse count is not
according to spec.

Reviewed-by: Jouni Högander <jouni.hogander at intel.com>

> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_aux.c | 32
> > +++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > index ad0aac707219..374492293392 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > @@ -118,6 +118,32 @@ static u32 skl_get_aux_clock_divider(struct
> > intel_dp *intel_dp, int index)
> >         return index ? 0 : 1;
> >  }
> >  
> > +static int intel_dp_aux_sync_len(void)
> > +{
> > +       int precharge = 16; /* 10-16 */
> > +       int preamble = 16;
> > +
> > +       return precharge + preamble;
> > +}
> > +
> > +static int intel_dp_aux_fw_sync_len(void)
> > +{
> > +       int precharge = 16; /* 10-16 */
> > +       int preamble = 8;
> > +
> > +       return precharge + preamble;
> > +}
> 
> What do you think if we move this into intel_dp_aux.h and use that to
> calculate io wake time and fast wake time in
> intel_psr.c:_compute_psr2_wake_time.
> 
> > +
> > +static int g4x_dp_aux_precharge_len(void)
> > +{
> > +       int precharge_min = 10;
> > +       int preamble = 16;
> > +
> > +       /* HW wants the length of the extra precharge in 2us units
> > */
> > +       return (intel_dp_aux_sync_len() -
> > +               precharge_min - preamble) / 2;
> > +}
> > +
> >  static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> >                                 int send_bytes,
> >                                 u32 aux_clock_divider)
> > @@ -140,7 +166,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp
> > *intel_dp,
> >                timeout |
> >                DP_AUX_CH_CTL_RECEIVE_ERROR |
> >                (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > -              (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> > +              (g4x_dp_aux_precharge_len() <<
> > DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> >                (aux_clock_divider <<
> > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
> >  }
> >  
> > @@ -164,8 +190,8 @@ static u32 skl_get_aux_send_ctl(struct intel_dp
> > *intel_dp,
> >               DP_AUX_CH_CTL_TIME_OUT_MAX |
> >               DP_AUX_CH_CTL_RECEIVE_ERROR |
> >               (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > -             DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(24) |
> > -             DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > +            
> > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len()) |
> > +            
> > DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
> >  
> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >                 ret |= DP_AUX_CH_CTL_TBT_IO;
>