[PATCH 2/2] drm/i915/gmbus: Add Wa_16025573575 for PTL for bit-bashing
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Thu Jul 3 06:05:54 UTC 2025
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.nautiyal at 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.
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
>>
More information about the Intel-xe
mailing list