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

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

On 7/11/2025 5:07 PM, Gustavo Sousa wrote:
> Quoting Ankit Nautiyal (2025-07-11 01:19:00-03:00)
>> As per Wa_16025573575 for PTL/WCL, set the GPIO masks bit before starting
>> bit-bashing and maintain value through the bit-bashing sequence. After the
>> bit-bashing sequence is done, clear the GPIO masks bits.
>>
>> v2:
>> -Use new helper for display workarounds. (Jani)
>> -Use a separate if-block for the workaround. (Gustavo)
>>
>> v3:
>> -Document the workaround details in intel_display_wa.c
>> -Extend the workaround to WCL too. (Gustavo)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>> .../gpu/drm/i915/display/intel_display_wa.c   | 12 +++++++
>> .../gpu/drm/i915/display/intel_display_wa.h   |  1 +
>> drivers/gpu/drm/i915/display/intel_gmbus.c    | 34 +++++++++++++++++--
>> 3 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c b/drivers/gpu/drm/i915/display/intel_display_wa.c
>> index 32719e2c6025..0dbcc1d272c7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
>> @@ -42,11 +42,23 @@ void intel_display_wa_apply(struct intel_display *display)
>>                  gen11_display_wa_apply(display);
>> }
>>
>> +/*
>> + * Wa_16025573575:
>> + * Fixes: Issue with bitbashing on Xe3 based platforms.
>> + * Workaround: Set masks bits in GPIO CTL and preserve it during bitbashing sequence.
>> + */
>> +static bool intel_display_needs_wa_16025573575(struct intel_display *display)
>> +{
>> +        return DISPLAY_VER(display) == 30 || DISPLAY_VERx100(display) == 3002;
> Using DISPLAY_VER(display) == 30 would match any verx100 between 3000
> and 3099. If in the future we come up with another variation of display
> version 30 that does not need the workaround, we would endup applying
> unnecessarily. So I think we should replace DISPLAY_VER(display) == 30
> with DISPLAY_VERx100(display) == 3000.

You are right Gustavo.

I will change this to DISPLAY_VERx100(display) == 3000 || 
DISPLAY_VERx100(display) == 3002.


>
>> +}
>> +
>> bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa, const char *name)
>> {
>>          switch (wa) {
>>          case INTEL_DISPLAY_WA_16023588340:
>>                  return intel_display_needs_wa_16023588340(display);
>> +        case INTEL_DISPLAY_WA_16025573575:
>> +                return intel_display_needs_wa_16025573575(display);
>>          default:
>>                  drm_WARN(display->drm, 1, "Missing Wa number: %s\n", name);
>>                  break;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h
>> index 8319e16eb460..aedea4cfa3ce 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h
>> @@ -23,6 +23,7 @@ bool intel_display_needs_wa_16023588340(struct intel_display *display);
>>
>> enum intel_display_wa {
>>          INTEL_DISPLAY_WA_16023588340,
>> +        INTEL_DISPLAY_WA_16025573575,
>> };
>>
>> bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa, const char *name);
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index 0d73f32fe7f1..6d6c3611d6c1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -39,6 +39,7 @@
>> #include "intel_de.h"
>> #include "intel_display_regs.h"
>> #include "intel_display_types.h"
>> +#include "intel_display_wa.h"
>> #include "intel_gmbus.h"
>> #include "intel_gmbus_regs.h"
>>
>> @@ -241,11 +242,18 @@ static u32 get_reserved(struct intel_gmbus *bus)
>> {
>>          struct intel_display *display = bus->display;
>>          u32 reserved = 0;
>> +        u32 preserve_bits = 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);
>> +                preserve_bits |= GPIO_DATA_PULLUP_DISABLE | GPIO_CLOCK_PULLUP_DISABLE;
>> +
>> +        /* Wa_16025573575: the masks bits need to be preserved through out */
>> +        if (intel_display_wa(display, 16025573575))
>> +                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;
> Maybe we can skip a register read if preserve_bits is zero?

Hmm.. that will be only true for platform i830 and i845g as of now.

But yes makes sense to retain the original intent for these platforms. 
So I will change to below:

static u32 get_reserved(struct intel_gmbus *bus)
{
         struct intel_display *display = bus->display;
         u32 preserve_bits = 0;

         if (display->platform.i830 || display->platform.i845g)
                 return 0;

         /* On most chips, these bits must be preserved in software. */
         preserve_bits |= GPIO_DATA_PULLUP_DISABLE | 
GPIO_CLOCK_PULLUP_DISABLE;

         /* Wa_16025573575: the masks bits need to be preserved through 
out */
         if (intel_display_wa(display, 16025573575))
                 preserve_bits |= GPIO_CLOCK_DIR_MASK | 
GPIO_CLOCK_VAL_MASK |
                                  GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;

         return intel_de_read_notrace(display, bus->gpio_reg) & 
preserve_bits;
}


Thanks & Regards,

Ankit

>
> --
> Gustavo Sousa
>
>>          return reserved;
>> }
>> @@ -308,6 +316,22 @@ static void set_data(void *data, int state_high)
>>          intel_de_posting_read(display, bus->gpio_reg);
>> }
>>
>> +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 (intel_display_wa(display, 16025573575))
>> +                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 (intel_display_wa(display, 16025573575))
>> +                ptl_handle_mask_bits(bus, false);
>> }
>>
>> static void
>> -- 
>> 2.45.2
>>