Quoting Nautiyal, Ankit K (2025-07-03 03:05:54-03:00)
>
>On 7/2/2025 6:41 PM, Gustavo Sousa wrote:
>> Quoting Ankit Nautiyal (2025-07-02 05:46:19-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.
>>>
>>> v2:
>>> -Use new helper for display workarounds. (Jani)
>>> -Use a separate if-block for the workaround. (Gustavo)
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nauti...@intel.com>
>>> ---
>>> .../gpu/drm/i915/display/intel_display_wa.c | 7 ++++
>>> .../gpu/drm/i915/display/intel_display_wa.h | 1 +
>>> drivers/gpu/drm/i915/display/intel_gmbus.c | 34 +++++++++++++++++--
>>> 3 files changed, 40 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 f5e8d58d9a68..12d1df5981f7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
>>> @@ -42,11 +42,18 @@ void intel_display_wa_apply(struct intel_display
>>> *display)
>>> gen11_display_wa_apply(display);
>>> }
>>>
>>> +static bool intel_display_needs_wa_16025573575(struct intel_display
>>> *display)
>>> +{
>>> + return DISPLAY_VER(display) == 30;
>> We should also check for 30.02.
>
>I was thinking to add a separate patch for this, but yeah can include in
>this patch as well.
>
>
>>
>>> +}
>>> +
>>> bool __intel_display_wa(struct intel_display *display, enum
>>> intel_display_wa wa)
>>> {
>>> 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);
>> While it makes sense to have function
>> intel_display_needs_wa_16023588340() (at least for now), I wonder if the
>> same could be said about intel_display_needs_wa_16025573575()...
>>
>> Maybe it would be simpler to just inline the conditions with a single
>> line here instead of adding 5 extra lines to the file.
>
>
>IMHO, it's better to keep __intel_display_wa() simple and uniform. In
>the future,
>
>some workarounds might involve complex conditions (such as checks for
>steppings,
>applicability to multiple platforms or variants)
>which could make the switch-case harder to read if inlined.
>
>Having dedicated functions like intel_display_needs_wa_xxxx() helps
>encapsulate that logic cleanly.
>
>Mixing inlined conditions with function calls would reduce consistency
>and readability.
Fair enough.
If you prefer to have a separate patch for WCL, then:
Reviewed-by: Gustavo Sousa <gustavo.so...@intel.com>
>
>
>Thanks & Regards,
>
>Ankit
>
>
>>
>> --
>> Gustavo Sousa
>>
>>> default:
>>> drm_WARN(display->drm, 1, "Missing Wa number: %d\n", wa);
>>> break;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h
>>> b/drivers/gpu/drm/i915/display/intel_display_wa.h
>>> index 146ee70d66f7..d3d241992e55 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);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c
>>> b/drivers/gpu/drm/i915/display/intel_gmbus.c
>>> index 0d73f32fe7f1..95cab11c9cde 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;
>>> +
>>> + /* PTL: 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;
>>>
>>> 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
>>>
