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

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

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.

>+}
>+
> 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?

--
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
>