Re: [PATCH] drm/i915/cdclk: Incorporate Xe3_LPD changes for CD2X divider

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Bhadane, Dnyaneshwar (2025-12-31 08:54:21-03:00)
>
>
>On 23-Dec-25 3:43 AM, Gustavo Sousa wrote:
>> On Xe3_LPD, there is no instruction to program the CD2X divider anymore
>> and the hardware is expected to always use the default value of 0b00,
>> meaning "divide by 1".
>> 
>> With that, the CDCLK_CTL register was changed so that:
>> 
>>    (1) The field "CD2X Divider Select" became a debug-only field.
>>        Because we are programming CDCLK_CTL with a direct write instead
>>        of read-modify-write operation, we still need to program "CD2X
>>        Divider Select" in order to keep the field from deviating from its
>>        default value.  Let's, however, throw a warning if we encounter a
>>        CDCLK value that would result in an unexpected value for that
>>        field.
>> 
>>    (2) The field "CD2X Pipe Select" has been removed. In fact, some
>>        debugging in a PTL machine showed that such field comes back as
>>        zero after writing a non-zero value to it.  As such, do not
>>        program it starting with Xe3_LPD.
>> 
>> Bspec: 68864, 69090
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_cdclk.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> index 0aa59d624095..684b8437951b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> @@ -1933,6 +1933,8 @@ static u32 bxt_cdclk_cd2x_pipe(struct intel_display *display, enum pipe pipe)
>>   static u32 bxt_cdclk_cd2x_div_sel(struct intel_display *display,
>>                                     int cdclk, int vco, u16 waveform)
>>   {
>> +        u32 ret;
>> +
>>           /* cdclk = vco / 2 / div{1,1.5,2,4} */
>>           switch (cdclk_divider(cdclk, vco, waveform)) {
>>           default:
>> @@ -1941,14 +1943,27 @@ static u32 bxt_cdclk_cd2x_div_sel(struct intel_display *display,
>>                   drm_WARN_ON(display->drm, vco != 0);
>>                   fallthrough;
>>           case 2:
>> -                return BXT_CDCLK_CD2X_DIV_SEL_1;
>> +                ret = BXT_CDCLK_CD2X_DIV_SEL_1;
>> +                break;
>>           case 3:
>> -                return BXT_CDCLK_CD2X_DIV_SEL_1_5;
>> +                ret = BXT_CDCLK_CD2X_DIV_SEL_1_5;
>> +                break;
>>           case 4:
>> -                return BXT_CDCLK_CD2X_DIV_SEL_2;
>> +                ret = BXT_CDCLK_CD2X_DIV_SEL_2;
>> +                break;
>>           case 8:
>> -                return BXT_CDCLK_CD2X_DIV_SEL_4;
>> +                ret = BXT_CDCLK_CD2X_DIV_SEL_4;
>> +                break;
>>           }
>> +
>> +        /*
>> +         * On Xe3_LPD onward, the expectation is to always have
>> +         * BXT_CDCLK_CD2X_DIV_SEL_1 as the default.
>> +         */
>> +        if (DISPLAY_VER(display) >= 30)
>> +                drm_WARN_ON(display->drm, ret != BXT_CDCLK_CD2X_DIV_SEL_1);
>> +
>> +        return ret;
>>   }
>>   
>>   static u16 cdclk_squash_waveform(struct intel_display *display,
>> @@ -2136,7 +2151,9 @@ static u32 bxt_cdclk_ctl(struct intel_display *display,
>>   
>>           waveform = cdclk_squash_waveform(display, cdclk);
>>   
>> -        val = bxt_cdclk_cd2x_div_sel(display, cdclk, vco, waveform) |
>> +        val = bxt_cdclk_cd2x_div_sel(display, cdclk, vco, waveform);
>> +
>> +        if (DISPLAY_VER(display) < 30)
>>                   bxt_cdclk_cd2x_pipe(display, pipe);
>Hey Gustavo,
>Please update this line, function’s return value should be ORed with val.
>
>i.e.        val |= bxt_cdclk_cd2x_pipe(display, pipe);

Oops... Thanks for catching that!

--
Gustavo Sousa

>
>BR,
>Dnyaneshwar
>>   
>>           /*
>> 
>> ---
>> base-commit: f2a0e58c77845e1a5cb6c549dc02b2670042e588
>> change-id: 20251222-xe3_lpd-no-cd2x-divider-48f9f0972f98
>> 
>> Best regards,
>> --
>> Gustavo Sousa <gustavo.sousa@intel.com>
>> 
>