[PATCH v2 0/3] drm/xe/i2c: alerts and controller enabling modifications

[PATCH v2 0/3] drm/xe/i2c: alerts and controller enabling modifications

[Heikki Krogerus]

Hi,

Changed since v1:
- Global header for the DesignWare I2C registers which meant a bit of
  patch refactoring.
- Selecting CONFIG_SMBUS in CONFIG_XE and handling smbus in xe_i2c.c instead of
  separate file.
- Storing the alert device to the client array and providing enum for the
  clients.
- Allowing other fields in the IC_ENABLE register to be updated except the
  Enable bit.
- Can't sleep in xe_i2c_disable() so using udelay().

v1: https://lore.kernel.org/lkml/20260622114759.3464047-1-heikki.krogerus@linux.intel.com/

This includes support for the SMBus alerts, and special handling for the
IC_ENABLE register.

Thanks,

Heikki Krogerus (3):
  i2c: designware: Global register definitions
  drm/xe/i2c: Handler for SMBus Alerts
  drm/xe/mcu_i2c: Take over control of the controller enabling

 MAINTAINERS                                |   1 +
 drivers/gpu/drm/xe/Kconfig                 |   1 +
 drivers/gpu/drm/xe/regs/xe_i2c_regs.h      |   2 +
 drivers/gpu/drm/xe/xe_i2c.c                | 108 +++++++++++++++++++--
 drivers/gpu/drm/xe/xe_i2c.h                |   6 ++
 drivers/i2c/busses/i2c-designware-common.c |   1 +
 drivers/i2c/busses/i2c-designware-core.h   |  84 +---------------
 drivers/i2c/busses/i2c-designware-master.c |   1 +
 include/linux/designware_i2c.h             | 107 ++++++++++++++++++++
 9 files changed, 222 insertions(+), 89 deletions(-)
 create mode 100644 include/linux/designware_i2c.h

-- 
2.50.1

[PATCH v2 1/3] i2c: designware: Global register definitions

[Heikki Krogerus]

Moving the register definitions to a global header file
include/linux/designware_i2c.h. That removes the need to
duplicate them in the adaptation layers for this driver
outside of drivers/i2c/busses/. There is at least one of
those in drivers/gpu/drm/xe/xe_i2c.c.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 MAINTAINERS                                |   1 +
 drivers/i2c/busses/i2c-designware-common.c |   1 +
 drivers/i2c/busses/i2c-designware-core.h   |  84 +---------------
 drivers/i2c/busses/i2c-designware-master.c |   1 +
 include/linux/designware_i2c.h             | 107 +++++++++++++++++++++
 5 files changed, 111 insertions(+), 83 deletions(-)
 create mode 100644 include/linux/designware_i2c.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4f176bcaa09e4..ca1dd05e79c65 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25947,6 +25947,7 @@ R:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 L:	linux-i2c@vger.kernel.org
 S:	Supported
 F:	drivers/i2c/busses/i2c-designware-*
+F:	include/linux/designware_i2c.h
 
 SYNOPSYS DESIGNWARE I2C DRIVER - AMDISP
 M:	Nirujogi Pratap <pratap.nirujogi@amd.com>
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 4dc57fd561703..1e5aba28e4942 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -16,6 +16,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/designware_i2c.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9d8d104cc3911..dfb6aa41e3f4a 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -11,6 +11,7 @@
 
 #include <linux/bits.h>
 #include <linux/completion.h>
+#include <linux/designware_i2c.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/irqreturn.h>
@@ -25,23 +26,6 @@
 						 I2C_FUNC_SMBUS_BLOCK_DATA | \
 						 I2C_FUNC_SMBUS_I2C_BLOCK)
 
-#define DW_IC_CON_MASTER			BIT(0)
-#define DW_IC_CON_SPEED_STD			(1 << 1)
-#define DW_IC_CON_SPEED_FAST			(2 << 1)
-#define DW_IC_CON_SPEED_HIGH			(3 << 1)
-#define DW_IC_CON_SPEED_MASK			GENMASK(2, 1)
-#define DW_IC_CON_10BITADDR_SLAVE		BIT(3)
-#define DW_IC_CON_10BITADDR_MASTER		BIT(4)
-#define DW_IC_CON_RESTART_EN			BIT(5)
-#define DW_IC_CON_SLAVE_DISABLE			BIT(6)
-#define DW_IC_CON_STOP_DET_IFADDRESSED		BIT(7)
-#define DW_IC_CON_TX_EMPTY_CTRL			BIT(8)
-#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL		BIT(9)
-#define DW_IC_CON_BUS_CLEAR_CTRL		BIT(11)
-
-#define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
-#define DW_IC_DATA_CMD_FIRST_DATA_BYTE		BIT(11)
-
 /*
  * Register access parameters
  */
@@ -55,65 +39,9 @@
 #define DW_IC_FIFO_RX_FIELD			GENMASK(15, 8)
 #define DW_IC_FIFO_MIN_DEPTH			2
 
-/*
- * Registers offset
- */
-#define DW_IC_CON				0x00
-#define DW_IC_TAR				0x04
-#define DW_IC_SAR				0x08
-#define DW_IC_DATA_CMD				0x10
-#define DW_IC_SS_SCL_HCNT			0x14
-#define DW_IC_SS_SCL_LCNT			0x18
-#define DW_IC_FS_SCL_HCNT			0x1c
-#define DW_IC_FS_SCL_LCNT			0x20
-#define DW_IC_HS_SCL_HCNT			0x24
-#define DW_IC_HS_SCL_LCNT			0x28
-#define DW_IC_INTR_STAT				0x2c
-#define DW_IC_INTR_MASK				0x30
-#define DW_IC_RAW_INTR_STAT			0x34
-#define DW_IC_RX_TL				0x38
-#define DW_IC_TX_TL				0x3c
-#define DW_IC_CLR_INTR				0x40
-#define DW_IC_CLR_RX_UNDER			0x44
-#define DW_IC_CLR_RX_OVER			0x48
-#define DW_IC_CLR_TX_OVER			0x4c
-#define DW_IC_CLR_RD_REQ			0x50
-#define DW_IC_CLR_TX_ABRT			0x54
-#define DW_IC_CLR_RX_DONE			0x58
-#define DW_IC_CLR_ACTIVITY			0x5c
-#define DW_IC_CLR_STOP_DET			0x60
-#define DW_IC_CLR_START_DET			0x64
-#define DW_IC_CLR_GEN_CALL			0x68
-#define DW_IC_ENABLE				0x6c
-#define DW_IC_STATUS				0x70
-#define DW_IC_TXFLR				0x74
-#define DW_IC_RXFLR				0x78
-#define DW_IC_SDA_HOLD				0x7c
-#define DW_IC_TX_ABRT_SOURCE			0x80
-#define DW_IC_ENABLE_STATUS			0x9c
-#define DW_IC_CLR_RESTART_DET			0xa8
-#define DW_IC_SMBUS_INTR_MASK			0xcc
-#define DW_IC_COMP_PARAM_1			0xf4
-#define DW_IC_COMP_VERSION			0xf8
 #define DW_IC_SDA_HOLD_MIN_VERS			0x3131312A /* "111*" == v1.11* */
-#define DW_IC_COMP_TYPE				0xfc
 #define DW_IC_COMP_TYPE_VALUE			0x44570140 /* "DW" + 0x0140 */
 
-#define DW_IC_INTR_RX_UNDER			BIT(0)
-#define DW_IC_INTR_RX_OVER			BIT(1)
-#define DW_IC_INTR_RX_FULL			BIT(2)
-#define DW_IC_INTR_TX_OVER			BIT(3)
-#define DW_IC_INTR_TX_EMPTY			BIT(4)
-#define DW_IC_INTR_RD_REQ			BIT(5)
-#define DW_IC_INTR_TX_ABRT			BIT(6)
-#define DW_IC_INTR_RX_DONE			BIT(7)
-#define DW_IC_INTR_ACTIVITY			BIT(8)
-#define DW_IC_INTR_STOP_DET			BIT(9)
-#define DW_IC_INTR_START_DET			BIT(10)
-#define DW_IC_INTR_GEN_CALL			BIT(11)
-#define DW_IC_INTR_RESTART_DET			BIT(12)
-#define DW_IC_INTR_MST_ON_HOLD			BIT(13)
-
 #define DW_IC_INTR_DEFAULT_MASK			(DW_IC_INTR_RX_FULL | \
 						 DW_IC_INTR_TX_ABRT | \
 						 DW_IC_INTR_STOP_DET)
@@ -123,16 +51,6 @@
 						 DW_IC_INTR_RX_UNDER | \
 						 DW_IC_INTR_RD_REQ)
 
-#define DW_IC_ENABLE_ENABLE			BIT(0)
-#define DW_IC_ENABLE_ABORT			BIT(1)
-
-#define DW_IC_STATUS_ACTIVITY			BIT(0)
-#define DW_IC_STATUS_TFE			BIT(2)
-#define DW_IC_STATUS_RFNE			BIT(3)
-#define DW_IC_STATUS_MASTER_ACTIVITY		BIT(5)
-#define DW_IC_STATUS_SLAVE_ACTIVITY		BIT(6)
-#define DW_IC_STATUS_MASTER_HOLD_TX_FIFO_EMPTY	BIT(7)
-
 #define DW_IC_SDA_HOLD_RX_SHIFT			16
 #define DW_IC_SDA_HOLD_RX_MASK			GENMASK(23, 16)
 
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index de929b91d5ead..3b113df0f28e0 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -12,6 +12,7 @@
 #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
 
 #include <linux/delay.h>
+#include <linux/designware_i2c.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
diff --git a/include/linux/designware_i2c.h b/include/linux/designware_i2c.h
new file mode 100644
index 0000000000000..53f37f18a7229
--- /dev/null
+++ b/include/linux/designware_i2c.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Synopsys DesignWare I2C register definitions
+ *
+ * Copyright (C) 2026, Intel Corporation
+ */
+
+#ifndef __LINUX_DESIGNWARE_I2C_H
+#define __LINUX_DESIGNWARE_I2C_H
+
+#include <linux/bits.h>
+
+/*
+ * Registers offset
+ */
+#define DW_IC_CON				0x00
+#define DW_IC_TAR				0x04
+#define DW_IC_SAR				0x08
+#define DW_IC_DATA_CMD				0x10
+#define DW_IC_SS_SCL_HCNT			0x14
+#define DW_IC_SS_SCL_LCNT			0x18
+#define DW_IC_FS_SCL_HCNT			0x1c
+#define DW_IC_FS_SCL_LCNT			0x20
+#define DW_IC_HS_SCL_HCNT			0x24
+#define DW_IC_HS_SCL_LCNT			0x28
+#define DW_IC_INTR_STAT				0x2c
+#define DW_IC_INTR_MASK				0x30
+#define DW_IC_RAW_INTR_STAT			0x34
+#define DW_IC_RX_TL				0x38
+#define DW_IC_TX_TL				0x3c
+#define DW_IC_CLR_INTR				0x40
+#define DW_IC_CLR_RX_UNDER			0x44
+#define DW_IC_CLR_RX_OVER			0x48
+#define DW_IC_CLR_TX_OVER			0x4c
+#define DW_IC_CLR_RD_REQ			0x50
+#define DW_IC_CLR_TX_ABRT			0x54
+#define DW_IC_CLR_RX_DONE			0x58
+#define DW_IC_CLR_ACTIVITY			0x5c
+#define DW_IC_CLR_STOP_DET			0x60
+#define DW_IC_CLR_START_DET			0x64
+#define DW_IC_CLR_GEN_CALL			0x68
+#define DW_IC_ENABLE				0x6c
+#define DW_IC_STATUS				0x70
+#define DW_IC_TXFLR				0x74
+#define DW_IC_RXFLR				0x78
+#define DW_IC_SDA_HOLD				0x7c
+#define DW_IC_TX_ABRT_SOURCE			0x80
+#define DW_IC_ENABLE_STATUS			0x9c
+#define DW_IC_CLR_RESTART_DET			0xa8
+#define DW_IC_SMBUS_INTR_STAT			0xc8
+#define DW_IC_SMBUS_INTR_MASK			0xcc
+#define DW_IC_CLR_SMBUS_INTR			0xd4
+#define DW_IC_COMP_PARAM_1			0xf4
+#define DW_IC_COMP_VERSION			0xf8
+#define DW_IC_COMP_TYPE				0xfc
+
+/* DW_IC_CON bits */
+#define DW_IC_CON_MASTER			BIT(0)
+#define DW_IC_CON_SPEED_STD			(1 << 1)
+#define DW_IC_CON_SPEED_FAST			(2 << 1)
+#define DW_IC_CON_SPEED_HIGH			(3 << 1)
+#define DW_IC_CON_SPEED_MASK			GENMASK(2, 1)
+#define DW_IC_CON_10BITADDR_SLAVE		BIT(3)
+#define DW_IC_CON_10BITADDR_MASTER		BIT(4)
+#define DW_IC_CON_RESTART_EN			BIT(5)
+#define DW_IC_CON_SLAVE_DISABLE			BIT(6)
+#define DW_IC_CON_STOP_DET_IFADDRESSED		BIT(7)
+#define DW_IC_CON_TX_EMPTY_CTRL			BIT(8)
+#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL		BIT(9)
+#define DW_IC_CON_BUS_CLEAR_CTRL		BIT(11)
+
+/* DW_IC_DATA_CMD bits */
+#define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
+#define DW_IC_DATA_CMD_FIRST_DATA_BYTE		BIT(11)
+
+/* DW_IC_INTR_* bits */
+#define DW_IC_INTR_RX_UNDER			BIT(0)
+#define DW_IC_INTR_RX_OVER			BIT(1)
+#define DW_IC_INTR_RX_FULL			BIT(2)
+#define DW_IC_INTR_TX_OVER			BIT(3)
+#define DW_IC_INTR_TX_EMPTY			BIT(4)
+#define DW_IC_INTR_RD_REQ			BIT(5)
+#define DW_IC_INTR_TX_ABRT			BIT(6)
+#define DW_IC_INTR_RX_DONE			BIT(7)
+#define DW_IC_INTR_ACTIVITY			BIT(8)
+#define DW_IC_INTR_STOP_DET			BIT(9)
+#define DW_IC_INTR_START_DET			BIT(10)
+#define DW_IC_INTR_GEN_CALL			BIT(11)
+#define DW_IC_INTR_RESTART_DET			BIT(12)
+#define DW_IC_INTR_MST_ON_HOLD			BIT(13)
+
+/* DW_IC_ENABLE bits */
+#define DW_IC_ENABLE_ENABLE			BIT(0)
+#define DW_IC_ENABLE_ABORT			BIT(1)
+
+/* DW_IC_STATUS bits */
+#define DW_IC_STATUS_ACTIVITY			BIT(0)
+#define DW_IC_STATUS_TFE			BIT(2)
+#define DW_IC_STATUS_RFNE			BIT(3)
+#define DW_IC_STATUS_MASTER_ACTIVITY		BIT(5)
+#define DW_IC_STATUS_SLAVE_ACTIVITY		BIT(6)
+#define DW_IC_STATUS_MASTER_HOLD_TX_FIFO_EMPTY	BIT(7)
+
+/* DW_IC_SMBUS_INTR_* bits */
+#define DW_IC_SMBUS_INTR_ALERT			BIT(10)
+
+#endif /* __LINUX_DESIGNWARE_I2C_H */
-- 
2.50.1

[PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[Heikki Krogerus]

Some devices that are attached to the I2C controller use the
SMBus Alert signal for example to inform the host about
thermal events, so registering the default SMBus Alert
device for them. The alert device makes sure that the alert
is processed and passed to the correct I2C client driver.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/gpu/drm/xe/Kconfig            |  1 +
 drivers/gpu/drm/xe/regs/xe_i2c_regs.h |  2 +
 drivers/gpu/drm/xe/xe_i2c.c           | 53 +++++++++++++++++++++++++--
 drivers/gpu/drm/xe/xe_i2c.h           |  5 +++
 4 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 4d7dcaff2b915..7045ba19943e3 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -46,6 +46,7 @@ config DRM_XE
 	select WANT_DEV_COREDUMP
 	select AUXILIARY_BUS
 	select REGMAP if I2C
+	select I2C_SMBUS
 	help
 	  Driver for Intel Xe2 series GPUs and later. Experimental support
 	  for Xe series is also available.
diff --git a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
index f2e455e2bfe45..37550e4a20f80 100644
--- a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
@@ -20,4 +20,6 @@
 #define I2C_CONFIG_CMD			XE_REG(I2C_CONFIG_SPACE_OFFSET + PCI_COMMAND)
 #define I2C_CONFIG_PMCSR		XE_REG(I2C_CONFIG_SPACE_OFFSET + 0x84)
 
+#define I2C_REG(reg)			XE_REG((reg) + I2C_MEM_SPACE_OFFSET)
+
 #endif /* _XE_I2C_REGS_H_ */
diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
index 706783863d07d..0495b561957a2 100644
--- a/drivers/gpu/drm/xe/xe_i2c.c
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -9,8 +9,10 @@
 #include <linux/array_size.h>
 #include <linux/container_of.h>
 #include <linux/device.h>
+#include <linux/designware_i2c.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
 #include <linux/ioport.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -30,6 +32,7 @@
 #include "xe_device.h"
 #include "xe_i2c.h"
 #include "xe_mmio.h"
+#include "xe_platform_types.h"
 #include "xe_sriov.h"
 #include "xe_survivability_mode.h"
 
@@ -46,6 +49,7 @@
  */
 
 static const char adapter_name[] = "i2c_designware";
+static struct i2c_smbus_alert_setup xe_i2c_smbus_setup;
 
 static const struct property_entry xe_i2c_adapter_properties[] = {
 	PROPERTY_ENTRY_STRING("compatible", "intel,xe-i2c"),
@@ -61,6 +65,37 @@ static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
 	val[1] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_POSTFIX);
 }
 
+static void xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
+{
+	u32 stat;
+
+	stat = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_SMBUS_INTR_STAT));
+	if (!stat)
+		return;
+
+	xe_mmio_write32(i2c->mmio, I2C_REG(DW_IC_CLR_SMBUS_INTR), stat);
+
+	if (stat & DW_IC_SMBUS_INTR_ALERT)
+		i2c_handle_smbus_alert(i2c->client[XE_I2C_CLIENT_ALERT]);
+}
+
+static int xe_i2c_register_smbus_alert(struct xe_i2c *i2c)
+{
+	struct xe_device *xe = kdev_to_xe_device(i2c->drm_dev);
+	struct i2c_client *alert;
+
+	if (xe->info.platform != XE_CRESCENTISLAND)
+		return 0;
+
+	alert = i2c_new_smbus_alert_device(i2c->adapter, &xe_i2c_smbus_setup);
+	if (IS_ERR(alert))
+		return PTR_ERR(alert);
+
+	i2c->client[XE_I2C_CLIENT_ALERT] = alert;
+
+	return 0;
+}
+
 static void xe_i2c_client_work(struct work_struct *work)
 {
 	struct xe_i2c *i2c = container_of(work, struct xe_i2c, work);
@@ -69,8 +104,13 @@ static void xe_i2c_client_work(struct work_struct *work)
 		.flags	= I2C_CLIENT_HOST_NOTIFY,
 		.addr	= i2c->ep.addr[1],
 	};
+	int ret;
 
-	i2c->client[0] = i2c_new_client_device(i2c->adapter, &info);
+	ret = xe_i2c_register_smbus_alert(i2c);
+	if (ret)
+		dev_warn(i2c->drm_dev, "failed to register smbus alert (%d)\n", ret);
+
+	i2c->client[XE_I2C_CLIENT_AMC] = i2c_new_client_device(i2c->adapter, &info);
 }
 
 static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void *data)
@@ -163,7 +203,7 @@ bool xe_i2c_present(struct xe_device *xe)
 
 static bool xe_i2c_irq_present(struct xe_device *xe)
 {
-	return xe->i2c && xe->i2c->adapter_irq;
+	return xe->i2c && (xe->i2c->adapter_irq || xe->i2c->client[XE_I2C_CLIENT_ALERT]);
 }
 
 /**
@@ -182,7 +222,10 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
 		return;
 
 	/* Forward interrupt to I2C adapter */
-	generic_handle_irq_safe(xe->i2c->adapter_irq);
+	if (xe->info.platform == XE_CRESCENTISLAND)
+		xe_i2c_handle_smbus_alert(xe->i2c);
+	else
+		generic_handle_irq_safe(xe->i2c->adapter_irq);
 
 	/* Deassert after I2C adapter clears the interrupt */
 	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
@@ -307,8 +350,10 @@ static void xe_i2c_remove(void *data)
 	struct xe_i2c *i2c = data;
 	unsigned int i;
 
-	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
+	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
 		i2c_unregister_device(i2c->client[i]);
+		i2c->client[i] = NULL;
+	}
 
 	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
 	xe_i2c_unregister_adapter(i2c);
diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
index 425d8160835f4..b3327db998708 100644
--- a/drivers/gpu/drm/xe/xe_i2c.h
+++ b/drivers/gpu/drm/xe/xe_i2c.h
@@ -23,6 +23,11 @@ struct xe_mmio;
 /* Endpoint Capabilities */
 #define XE_I2C_EP_CAP_IRQ		BIT(0)
 
+enum XE_I2C_CLIENT {
+	XE_I2C_CLIENT_AMC,
+	XE_I2C_CLIENT_ALERT,
+};
+
 struct xe_i2c_endpoint {
 	u8 cookie;
 	u8 capabilities;
-- 
2.50.1

[PATCH v2 3/3] drm/xe/mcu_i2c: Take over control of the controller enabling

[Heikki Krogerus]

Some platforms make an assumption that the i2c controller's
enabled state indicates also the power state of the
controller. This can create a problem when the controller is
in disabled state, because the hardware may assume
incorrectly that it is then also in low-power state.

To fix this, the controller is kept enabled by taking over
the IC_ENABLE register. The controller has to be disabled
when the configuration is updated and when the target
address or the slave address are assigned, so disabling it
when IC_CON, IC_TAR or IC_SAR registers are programmed, and
then re-enabling it again.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_i2c.c | 55 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/xe/xe_i2c.h |  1 +
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
index 0495b561957a2..e2d8e77653cf2 100644
--- a/drivers/gpu/drm/xe/xe_i2c.c
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -8,6 +8,7 @@
 #include <drm/drm_print.h>
 #include <linux/array_size.h>
 #include <linux/container_of.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/designware_i2c.h>
 #include <linux/err.h>
@@ -294,11 +295,40 @@ static void xe_i2c_remove_irq(struct xe_i2c *i2c)
 	irq_domain_remove(i2c->irqdomain);
 }
 
+/* See "Disabling DW_apb_i2c" in the DesignWare DW_abp_i2c databook. */
+static void xe_i2c_disable(struct xe_i2c *i2c)
+{
+	int timeout = 100;
+	u32 status;
+
+	xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), 1, 0);
+
+	do {
+		status = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_ENABLE_STATUS));
+		if (!(status & 1))
+			return;
+		/* Can't sleep here. */
+		udelay(25);
+	} while (timeout--);
+
+	dev_warn(&i2c->adapter->dev, "timeout in disabling adapter\n");
+}
+
 static int xe_i2c_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct xe_i2c *i2c = context;
 
-	*val = xe_mmio_read32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET));
+	*val = xe_mmio_read32(i2c->mmio, I2C_REG(reg));
+
+	switch (reg) {
+	case DW_IC_ENABLE:
+	case DW_IC_ENABLE_STATUS:
+		FIELD_MODIFY(DW_IC_ENABLE_ENABLE, val,
+			     i2c->ic_enable & DW_IC_ENABLE_ENABLE);
+		break;
+	default:
+		break;
+	}
 
 	return 0;
 }
@@ -307,7 +337,28 @@ static int xe_i2c_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct xe_i2c *i2c = context;
 
-	xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
+	switch (reg) {
+	case DW_IC_CON:
+	case DW_IC_TAR:
+	case DW_IC_SAR:
+		/* Disable the controller. */
+		xe_i2c_disable(i2c);
+
+		/* Write the register. */
+		xe_mmio_write32(i2c->mmio, I2C_REG(reg), val);
+
+		/* Enable the controller. */
+		xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), 0, 1);
+		break;
+	case DW_IC_ENABLE:
+		i2c->ic_enable = val;
+		/* Other fields can be updated except the enable bit. */
+		val |= DW_IC_ENABLE_ENABLE;
+		fallthrough;
+	default:
+		xe_mmio_write32(i2c->mmio, I2C_REG(reg), val);
+		break;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
index b3327db998708..231e36df09017 100644
--- a/drivers/gpu/drm/xe/xe_i2c.h
+++ b/drivers/gpu/drm/xe/xe_i2c.h
@@ -39,6 +39,7 @@ struct xe_i2c {
 	struct platform_device *pdev;
 	struct i2c_adapter *adapter;
 	struct i2c_client *client[XE_I2C_MAX_CLIENTS];
+	unsigned int ic_enable;
 
 	struct notifier_block bus_notifier;
 	struct work_struct work;
-- 
2.50.1

Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unconditional dereference of a potentially NULL I2C client pointer when handling SMBus alerts.
- [High] Normal I2C adapter interrupts are not propagated to the adapter driver on the XE_CRESCENTISLAND platform, breaking standard I2C transfers.
- [High] Writing to a read-to-clear interrupt status register will fail to clear the interrupt, leading to an interrupt storm.
- [Low] DRM_XE unconditionally selects I2C_SMBUS, causing unmet dependencies and build failures when I2C is not enabled.

Pre-existing issues:
- [High] Missing `cancel_work_sync()` in the device cleanup path allows pending asynchronous work to access freed memory.
--

commit 99e285e8af69e8587ef5c6dca5bee3189103c64e
Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: drm/xe/i2c: Handler for SMBus Alerts

This commit adds support for handling SMBus alerts in the XE I2C driver.
It introduces a new client for the alert device and an interrupt handling
path to forward alerts to the SMBus core.

> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -46,6 +46,7 @@ config DRM_XE
>  	select WANT_DEV_COREDUMP
>  	select AUXILIARY_BUS
>  	select REGMAP if I2C
> +	select I2C_SMBUS

[Severity: Low]
Does this select statement need to be guarded conditionally or have a
dependency on I2C added to DRM_XE?

Selecting I2C_SMBUS unconditionally without inheriting its dependency on I2C
can cause unmet dependency warnings and build failures when I2C is disabled.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
[ ... ]
> @@ -61,6 +65,37 @@ static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
>  	val[1] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_POSTFIX);
>  }
>  
> +static void xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
> +{
> +	u32 stat;
> +
> +	stat = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_SMBUS_INTR_STAT));
> +	if (!stat)
> +		return;
> +
> +	xe_mmio_write32(i2c->mmio, I2C_REG(DW_IC_CLR_SMBUS_INTR), stat);

[Severity: High]
Will this successfully clear the interrupt?

Synopsys DesignWare I2C DW_IC_CLR_* registers typically use read-to-clear
semantics. Writing to this register might not clear the hardware interrupt,
potentially leading to an interrupt storm when the ISR returns.

> +
> +	if (stat & DW_IC_SMBUS_INTR_ALERT)
> +		i2c_handle_smbus_alert(i2c->client[XE_I2C_CLIENT_ALERT]);

[Severity: Critical]
Is it possible for i2c->client[XE_I2C_CLIENT_ALERT] to be NULL here?

The client registration happens asynchronously in xe_i2c_client_work(). If an
SMBus alert hardware interrupt fires before the async workqueue executes, or
if the client registration fails, passing a NULL pointer to
i2c_handle_smbus_alert() will cause a NULL pointer dereference.

> +}
[ ... ]
> @@ -182,7 +222,10 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
>  		return;
>  
>  	/* Forward interrupt to I2C adapter */
> -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> +	if (xe->info.platform == XE_CRESCENTISLAND)
> +		xe_i2c_handle_smbus_alert(xe->i2c);
> +	else
> +		generic_handle_irq_safe(xe->i2c->adapter_irq);

[Severity: High]
Does this prevent regular I2C adapter interrupts from being handled on the
XE_CRESCENTISLAND platform?

By placing generic_handle_irq_safe() in the else branch, it seems normal I2C
transfers might time out and fail because their hardware interrupts are
entirely skipped on this platform.

>  	/* Deassert after I2C adapter clears the interrupt */
>  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
[ ... ]
> @@ -307,8 +350,10 @@ static void xe_i2c_remove(void *data)
>  	struct xe_i2c *i2c = data;
>  	unsigned int i;
>  
> -	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> +	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
>  		i2c_unregister_device(i2c->client[i]);
> +		i2c->client[i] = NULL;
> +	}
>  
>  	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
>  	xe_i2c_unregister_adapter(i2c);

[Severity: High]
This is a pre-existing issue, but is there a missing call to
cancel_work_sync() in this teardown path?

The asynchronous workqueue item i2c->work is scheduled when the I2C adapter
is added. If this device is removed while the work is still pending or
running, xe_i2c_client_work() could dereference freed memory after
xe_i2c_remove() completes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625125939.429078-1-heikki.krogerus@linux.intel.com?part=2

Re: [PATCH v2 3/3] drm/xe/mcu_i2c: Take over control of the controller enabling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Faking the disabled state of the I2C controller while keeping it physically enabled causes an unhandled IRQ storm if the hardware generates an unmasked interrupt.
- [Critical] Bypassing the core driver's wait loop shifts the wait into a much shorter atomic context, causing Target Address (TAR) writes to be silently dropped if the bus is active.
- [High] I2C timing and FIFO configuration registers are silently ignored by the hardware because the controller is forcibly kept enabled and the regmap interceptor fails to temporarily disable it.
- [High] Dereferencing `i2c->adapter` in `xe_i2c_disable` causes a NULL pointer dereference if a timeout occurs during device probe.
--

commit ef1d705e3f064c441441c723d0ee6ac370743a6b
Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>

drm/xe/mcu_i2c: Take over control of the controller enabling

This commit modifies the mcu_i2c driver to maintain the I2C controller
in an enabled state to prevent incorrect low-power state assumptions by
the hardware. It intercepts writes to the IC_ENABLE register and
temporarily disables the controller when programming configuration
and target address registers.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index 0495b561957a2..e2d8e77653cf2 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
[ ... ]
> @@ -294,11 +295,40 @@ static void xe_i2c_remove_irq(struct xe_i2c *i2c)
>  	irq_domain_remove(i2c->irqdomain);
>  }
>  
> +/* See "Disabling DW_apb_i2c" in the DesignWare DW_abp_i2c databook. */
> +static void xe_i2c_disable(struct xe_i2c *i2c)
> +{
> +	int timeout = 100;
> +	u32 status;
> +
> +	xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), 1, 0);
> +
> +	do {
> +		status = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_ENABLE_STATUS));
> +		if (!(status & 1))
> +			return;
> +		/* Can't sleep here. */
> +		udelay(25);
> +	} while (timeout--);
> +
> +	dev_warn(&i2c->adapter->dev, "timeout in disabling adapter\n");

[Severity: High]
If a timeout occurs during device probe when i2c_dw_init() writes to
DW_IC_CON, does this dereference a NULL pointer?

During probe, xe_i2c_write() temporarily disables the hardware via
xe_i2c_disable(). If this times out, it logs a warning using i2c->adapter,
but i2c->adapter appears to only be assigned later during the
BUS_NOTIFY_ADD_DEVICE notifier.

> +}
> +
>  static int xe_i2c_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct xe_i2c *i2c = context;
>  
> -	*val = xe_mmio_read32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET));
> +	*val = xe_mmio_read32(i2c->mmio, I2C_REG(reg));
> +
> +	switch (reg) {
> +	case DW_IC_ENABLE:
> +	case DW_IC_ENABLE_STATUS:
> +		FIELD_MODIFY(DW_IC_ENABLE_ENABLE, val,
> +			     i2c->ic_enable & DW_IC_ENABLE_ENABLE);
> +		break;

[Severity: Critical]
Will this spoofed status cause an unhandled interrupt storm if the hardware
generates an unmasked interrupt?

Because the physical controller remains enabled while xe_i2c_read() returns
a faked disabled status, there is a window where an interrupt can fire.
When i2c_dw_isr_master() runs, it reads the spoofed disabled status,
assumes the interrupt is spurious, and immediately returns IRQ_NONE
without clearing it. Does this lead to the hardware continuously asserting
the interrupt?

> +	default:
> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -307,7 +337,28 @@ static int xe_i2c_write(void *context, unsigned int reg, unsigned int val)
>  {
>  	struct xe_i2c *i2c = context;
>  
> -	xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
> +	switch (reg) {
> +	case DW_IC_CON:
> +	case DW_IC_TAR:
> +	case DW_IC_SAR:
> +		/* Disable the controller. */
> +		xe_i2c_disable(i2c);
> +
> +		/* Write the register. */
> +		xe_mmio_write32(i2c->mmio, I2C_REG(reg), val);

[Severity: Critical]
Is it possible for Target Address writes to be silently dropped if the bus
is active and disabling takes longer than 2.5ms?

Since xe_i2c_read() spoofs DW_IC_ENABLE_STATUS, the core dw_i2c driver's
sleep-capable wait loop in __i2c_dw_disable() exits immediately. This shifts
the wait burden to xe_i2c_disable() which runs under a regmap spinlock with
a 2.5ms maximum timeout (100 iterations of udelay(25)).

If xe_i2c_disable() times out and returns silently, xe_i2c_write() blindly
writes the new target address (DW_IC_TAR) while the hardware is still
physically enabled. Because the hardware discards this write, could
subsequent data payloads be erroneously sent to the previously addressed
slave device?

> +
> +		/* Enable the controller. */
> +		xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), 0, 1);
> +		break;
> +	case DW_IC_ENABLE:
> +		i2c->ic_enable = val;
> +		/* Other fields can be updated except the enable bit. */
> +		val |= DW_IC_ENABLE_ENABLE;
> +		fallthrough;
> +	default:
> +		xe_mmio_write32(i2c->mmio, I2C_REG(reg), val);
> +		break;

[Severity: High]
Are timing and FIFO registers silently ignored by the hardware when they fall
through to this default case?

The DesignWare I2C Databook requires that timing registers
(e.g., DW_IC_SS_SCL_HCNT) and FIFO thresholds are written when the
controller is disabled. Since this patch forces the hardware to remain
enabled, any writes to registers that fall through to the default case are
executed while the hardware is still enabled, which causes the hardware to
silently discard them.

> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625125939.429078-1-heikki.krogerus@linux.intel.com?part=3

Re: [PATCH v2 1/3] i2c: designware: Global register definitions

[Andy Shevchenko]

On Thu, Jun 25, 2026 at 02:59:37PM +0200, Heikki Krogerus wrote:
> Moving the register definitions to a global header file
> include/linux/designware_i2c.h. That removes the need to
> duplicate them in the adaptation layers for this driver
> outside of drivers/i2c/busses/. There is at least one of
> those in drivers/gpu/drm/xe/xe_i2c.c.

OK!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[Raag Jadav]

On Thu, Jun 25, 2026 at 02:59:38PM +0200, Heikki Krogerus wrote:
> Some devices that are attached to the I2C controller use the
> SMBus Alert signal for example to inform the host about
> thermal events, so registering the default SMBus Alert
> device for them. The alert device makes sure that the alert
> is processed and passed to the correct I2C client driver.

...

> +static int xe_i2c_register_smbus_alert(struct xe_i2c *i2c)
> +{
> +	struct xe_device *xe = kdev_to_xe_device(i2c->drm_dev);
> +	struct i2c_client *alert;
> +
> +	if (xe->info.platform != XE_CRESCENTISLAND)

Rather, introduce a has_i2c_smbus flag in xe_device_types.h and keep
things platform agnostic. I think you'll also need it to early return
from xe_i2c_create_irq().

> +		return 0;
> +
> +	alert = i2c_new_smbus_alert_device(i2c->adapter, &xe_i2c_smbus_setup);
> +	if (IS_ERR(alert))
> +		return PTR_ERR(alert);
> +
> +	i2c->client[XE_I2C_CLIENT_ALERT] = alert;
> +
> +	return 0;
> +}

...

> @@ -182,7 +222,10 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
>  		return;
>  
>  	/* Forward interrupt to I2C adapter */
> -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> +	if (xe->info.platform == XE_CRESCENTISLAND)

Ditto.

Raag

> +		xe_i2c_handle_smbus_alert(xe->i2c);
> +	else
> +		generic_handle_irq_safe(xe->i2c->adapter_irq);

Re: [PATCH v2 3/3] drm/xe/mcu_i2c: Take over control of the controller enabling

[Raag Jadav]

On Thu, Jun 25, 2026 at 02:59:39PM +0200, Heikki Krogerus wrote:
> Some platforms make an assumption that the i2c controller's
> enabled state indicates also the power state of the
> controller. This can create a problem when the controller is
> in disabled state, because the hardware may assume
> incorrectly that it is then also in low-power state.
> 
> To fix this, the controller is kept enabled by taking over
> the IC_ENABLE register. The controller has to be disabled
> when the configuration is updated and when the target
> address or the slave address are assigned, so disabling it
> when IC_CON, IC_TAR or IC_SAR registers are programmed, and
> then re-enabling it again.

I don't have enough context here, so I'll leave this one to the
maintainers.

Raag

Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[Heikki Krogerus]

On Tue, Jun 30, 2026 at 10:39:39AM +0200, Raag Jadav wrote:
> On Thu, Jun 25, 2026 at 02:59:38PM +0200, Heikki Krogerus wrote:
> > Some devices that are attached to the I2C controller use the
> > SMBus Alert signal for example to inform the host about
> > thermal events, so registering the default SMBus Alert
> > device for them. The alert device makes sure that the alert
> > is processed and passed to the correct I2C client driver.
> 
> ...
> 
> > +static int xe_i2c_register_smbus_alert(struct xe_i2c *i2c)
> > +{
> > +	struct xe_device *xe = kdev_to_xe_device(i2c->drm_dev);
> > +	struct i2c_client *alert;
> > +
> > +	if (xe->info.platform != XE_CRESCENTISLAND)
> 
> Rather, introduce a has_i2c_smbus flag in xe_device_types.h and keep
> things platform agnostic. I think you'll also need it to early return
> from xe_i2c_create_irq().

OK. Feature flags are nice.

Thanks!

-- 
heikki

Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[Raag Jadav]

On Thu, Jun 25, 2026 at 02:59:38PM +0200, Heikki Krogerus wrote:
> Some devices that are attached to the I2C controller use the
> SMBus Alert signal for example to inform the host about
> thermal events, so registering the default SMBus Alert
> device for them. The alert device makes sure that the alert
> is processed and passed to the correct I2C client driver.

...

> +enum XE_I2C_CLIENT {
> +	XE_I2C_CLIENT_AMC,
> +	XE_I2C_CLIENT_ALERT,

Forgot to add, with this I think you can move XE_I2C_MAX_CLIENTS define
here.

Raag

> +};

Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[Heikki Krogerus]

On Tue, Jun 30, 2026 at 12:28:08PM +0200, Raag Jadav wrote:
> On Thu, Jun 25, 2026 at 02:59:38PM +0200, Heikki Krogerus wrote:
> > Some devices that are attached to the I2C controller use the
> > SMBus Alert signal for example to inform the host about
> > thermal events, so registering the default SMBus Alert
> > device for them. The alert device makes sure that the alert
> > is processed and passed to the correct I2C client driver.
> 
> ...
> 
> > +enum XE_I2C_CLIENT {
> > +	XE_I2C_CLIENT_AMC,
> > +	XE_I2C_CLIENT_ALERT,
> 
> Forgot to add, with this I think you can move XE_I2C_MAX_CLIENTS define
> here.

Ah, makes sense. I'll fix that.

Thanks!

-- 
heikki