Re: [PATCH] drm/i915/gmbus: Add Wa_16025573575 for PTL for bit-bashing

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 6/12/2025 5:30 PM, Gustavo Sousa wrote:
> Quoting Ankit Nautiyal (2025-06-12 04:53:30-03:00)
>> As per Wa_16025573575 for PTL, set the GPIO masks bit before starting
>> bit-bashing and maintain value through the bit-bashing sequence.
>> After bit-bashing sequence is done, clear the GPIO masks bits.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_gmbus.c | 38 +++++++++++++++++++---
>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index 0d73f32fe7f1..c96167c70228 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -242,10 +242,17 @@ static u32 get_reserved(struct intel_gmbus *bus)
>>          struct intel_display *display = bus->display;
>>          u32 reserved = 0;
>>
>> -        /* On most chips, these bits must be preserved in software. */
>> -        if (!display->platform.i830 && !display->platform.i845g)
>> -                reserved = intel_de_read_notrace(display, bus->gpio_reg) &
>> -                        (GPIO_DATA_PULLUP_DISABLE | GPIO_CLOCK_PULLUP_DISABLE);
>> +        if (!display->platform.i830 && !display->platform.i845g) {
> I believe the stuff specific to Wa_16025573575 could be done in a
> separate "if" statement. The conditions for Wa_16025573575 already imply
> (!display->platform.i830 && !display->platform.i845g), and using in a
> separate "if" statement reduces one indentation level :-)

Makes sense, can make a separate if-block for this.


>
> That would mean moving preserve_bits one level up, making it available
> in the function's scope and doing the registers read conditioned on
> preserve_bits being non-zero.
>
>> +                /* On most chips, these bits must be preserved in software. */
>> +                u32 preserve_bits = GPIO_DATA_PULLUP_DISABLE | GPIO_CLOCK_PULLUP_DISABLE;
>> +
>> +                /* PTL: Wa_16025573575: the masks bits need to be preserved through out */
>> +                if (DISPLAY_VER(display) == 30)
> Instead of open-coding the conditions for the workaround in three
> different places in the file, I think we should have a function
> needs_wa_16025573575() and use it.
>
> Also, note that the workaround is also required for WCL (display version
> 30.02), and we would then include that in needs_wa_16025573575().

I agree, will make a separate function needs_wa_16025573575().

Thanks for the suggestions. I will make suggested changes in next revision.

Regards,

Ankit

>
> --
> Gustavo Sousa
>
>> +                        preserve_bits |= GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK |
>> +                                         GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
>> +
>> +                reserved = intel_de_read_notrace(display, bus->gpio_reg) & preserve_bits;
>> +        }
>>
>>          return reserved;
>> }
>> @@ -308,6 +315,23 @@ static void set_data(void *data, int state_high)
>>          intel_de_posting_read(display, bus->gpio_reg);
>> }
>>
>> +/* PTL: Wa_16025573575 */
>> +static void
>> +ptl_handle_mask_bits(struct intel_gmbus *bus, bool set)
>> +{
>> +        struct intel_display *display = bus->display;
>> +        u32 reg_val = intel_de_read_notrace(display, bus->gpio_reg);
>> +        u32 mask_bits = GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK |
>> +                        GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
>> +        if (set)
>> +                reg_val |= mask_bits;
>> +        else
>> +                reg_val &= ~mask_bits;
>> +
>> +        intel_de_write_notrace(display, bus->gpio_reg, reg_val);
>> +        intel_de_posting_read(display, bus->gpio_reg);
>> +}
>> +
>> static int
>> intel_gpio_pre_xfer(struct i2c_adapter *adapter)
>> {
>> @@ -319,6 +343,9 @@ intel_gpio_pre_xfer(struct i2c_adapter *adapter)
>>          if (display->platform.pineview)
>>                  pnv_gmbus_clock_gating(display, false);
>>
>> +        if (DISPLAY_VER(display) == 30)
>> +                ptl_handle_mask_bits(bus, true);
>> +
>>          set_data(bus, 1);
>>          set_clock(bus, 1);
>>          udelay(I2C_RISEFALL_TIME);
>> @@ -336,6 +363,9 @@ intel_gpio_post_xfer(struct i2c_adapter *adapter)
>>
>>          if (display->platform.pineview)
>>                  pnv_gmbus_clock_gating(display, true);
>> +
>> +        if (DISPLAY_VER(display) == 30)
>> +                ptl_handle_mask_bits(bus, false);
>> }
>>
>> static void
>> -- 
>> 2.45.2
>>