[PATCH v5 0/4] drm/xe: i2c support

[PATCH v5 0/4] drm/xe: i2c support

[Heikki Krogerus]

Hi,

Changed since v4:
- Cleanups requested by Andy.
- Casting the PCI power modes to make sparse happy - Raag.
- Limiting the use of this thing to Battlemage again. But I'm not sure
  if this is the correct thing to do.

Changed since v3:
- Cleanups as requested by Rodrigo.
- The licence is now changed, but still need confirmation for it!

Changed since v2:
- Added dependency on regmap when i2c is enabled.

Changed since v1:
- Now rebased on top of drm-tip.
- No longer ignoring errors from xe_i2c_probe().
- Cleanups pointed out by Lucas.

I've also included followup patches from Raag and Riana to this
series.

Original cover letter:

Some of the future GPUs will provide access to the on-board Synopsys
DesignWare I2C host adapter. The i2c is used to connect various
microcontrollers. The initially supported microcontroller unit is
called Add-In Management Controller (AMC).

Thanks,

Heikki Krogerus (2):
  i2c: designware: Add quirk for Intel Xe
  drm/xe: Support for I2C attached MCUs

Raag Jadav (1):
  drm/xe/pm: Wire up suspend/resume for I2C controller

Riana Tauro (1):
  drm/xe/xe_i2c: Add support for i2c in survivability mode

 drivers/gpu/drm/xe/Kconfig                  |   1 +
 drivers/gpu/drm/xe/Makefile                 |   1 +
 drivers/gpu/drm/xe/regs/xe_i2c_regs.h       |  20 ++
 drivers/gpu/drm/xe/regs/xe_irq_regs.h       |   1 +
 drivers/gpu/drm/xe/regs/xe_pmt.h            |   2 +-
 drivers/gpu/drm/xe/regs/xe_regs.h           |   2 +
 drivers/gpu/drm/xe/xe_device.c              |   5 +
 drivers/gpu/drm/xe/xe_device_types.h        |   4 +
 drivers/gpu/drm/xe/xe_i2c.c                 | 329 ++++++++++++++++++++
 drivers/gpu/drm/xe/xe_i2c.h                 |  62 ++++
 drivers/gpu/drm/xe/xe_irq.c                 |   2 +
 drivers/gpu/drm/xe/xe_pm.c                  |   9 +
 drivers/gpu/drm/xe/xe_survivability_mode.c  |  23 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |  18 +-
 14 files changed, 464 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/regs/xe_i2c_regs.h
 create mode 100644 drivers/gpu/drm/xe/xe_i2c.c
 create mode 100644 drivers/gpu/drm/xe/xe_i2c.h

-- 
2.47.2

[PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Heikki Krogerus]

The regmap is coming from the parent also in case of Xe
GPUs. Reusing the Wangxun quirk for that.

Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 879719e91df2..a35e4c64a1d4 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -101,7 +101,7 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
 }
 #endif
 
-static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
+static int dw_i2c_get_parent_regmap(struct dw_i2c_dev *dev)
 {
 	dev->map = dev_get_regmap(dev->dev->parent, NULL);
 	if (!dev->map)
@@ -123,12 +123,15 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
 	struct platform_device *pdev = to_platform_device(dev->dev);
 	int ret;
 
+	if (device_is_compatible(dev->dev, "intel,xe-i2c"))
+		return dw_i2c_get_parent_regmap(dev);
+
 	switch (dev->flags & MODEL_MASK) {
 	case MODEL_BAIKAL_BT1:
 		ret = bt1_i2c_request_regs(dev);
 		break;
 	case MODEL_WANGXUN_SP:
-		ret = txgbe_i2c_request_regs(dev);
+		ret = dw_i2c_get_parent_regmap(dev);
 		break;
 	default:
 		dev->base = devm_platform_ioremap_resource(pdev, 0);
@@ -205,25 +208,28 @@ static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
 
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
+	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
 	struct device *device = &pdev->dev;
 	struct i2c_adapter *adap;
 	struct dw_i2c_dev *dev;
 	int irq, ret;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
+	irq = platform_get_irq_optional(pdev, 0);
+	if (irq == -ENXIO)
+		flags |= ACCESS_POLLING;
+	else if (irq < 0)
 		return irq;
 
 	dev = devm_kzalloc(device, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
-	dev->flags = (uintptr_t)device_get_match_data(device);
 	if (device_property_present(device, "wx,i2c-snps-model"))
-		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
+		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
 
 	dev->dev = device;
 	dev->irq = irq;
+	dev->flags = flags;
 	platform_set_drvdata(pdev, dev);
 
 	ret = dw_i2c_plat_request_regs(dev);
-- 
2.47.2

[PATCH v5 2/4] drm/xe: Support for I2C attached MCUs

[Heikki Krogerus]

Adding adaption/glue layer where the I2C host adapter
(Synopsys DesignWare I2C adapter) and the I2C clients (the
microcontroller units) are enumerated.

The microcontroller units (MCU) that are attached to the GPU
depend on the OEM. The initially supported MCU will be the
Add-In Management Controller (AMC).

Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/gpu/drm/xe/Kconfig            |   1 +
 drivers/gpu/drm/xe/Makefile           |   1 +
 drivers/gpu/drm/xe/regs/xe_i2c_regs.h |  15 ++
 drivers/gpu/drm/xe/regs/xe_irq_regs.h |   1 +
 drivers/gpu/drm/xe/regs/xe_pmt.h      |   2 +-
 drivers/gpu/drm/xe/regs/xe_regs.h     |   2 +
 drivers/gpu/drm/xe/xe_device.c        |   5 +
 drivers/gpu/drm/xe/xe_device_types.h  |   4 +
 drivers/gpu/drm/xe/xe_i2c.c           | 300 ++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_i2c.h           |  58 +++++
 drivers/gpu/drm/xe/xe_irq.c           |   2 +
 11 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/xe/regs/xe_i2c_regs.h
 create mode 100644 drivers/gpu/drm/xe/xe_i2c.c
 create mode 100644 drivers/gpu/drm/xe/xe_i2c.h

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 553c29e1030b..eb361c202581 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -44,6 +44,7 @@ config DRM_XE
 	select WANT_DEV_COREDUMP
 	select AUXILIARY_BUS
 	select HMM_MIRROR
+	select REGMAP if I2C
 	help
 	  Driver for Intel Xe2 series GPUs and later. Experimental support
 	  for Xe series is also available.
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index eee6bac01a00..4b8a8fcd3959 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -125,6 +125,7 @@ xe-y += xe_bb.o \
 	xe_wait_user_fence.o \
 	xe_wopcm.o
 
+xe-$(CONFIG_I2C)	+= xe_i2c.o
 xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
 xe-$(CONFIG_DRM_XE_GPUSVM) += xe_svm.o
 
diff --git a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
new file mode 100644
index 000000000000..92dae4487614
--- /dev/null
+++ b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+#ifndef _XE_I2C_REGS_H_
+#define _XE_I2C_REGS_H_
+
+#include "xe_reg_defs.h"
+#include "xe_regs.h"
+
+#define I2C_BRIDGE_OFFSET		(SOC_BASE + 0xd9000)
+#define I2C_CONFIG_SPACE_OFFSET		(SOC_BASE + 0xf6000)
+#define I2C_MEM_SPACE_OFFSET		(SOC_BASE + 0xf7400)
+
+#define REG_SG_REMAP_ADDR_PREFIX	XE_REG(SOC_BASE + 0x0164)
+#define REG_SG_REMAP_ADDR_POSTFIX	XE_REG(SOC_BASE + 0x0168)
+
+#endif /* _XE_I2C_REGS_H_ */
diff --git a/drivers/gpu/drm/xe/regs/xe_irq_regs.h b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
index f0ecfcac4003..13635e4331d4 100644
--- a/drivers/gpu/drm/xe/regs/xe_irq_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
@@ -19,6 +19,7 @@
 #define   MASTER_IRQ				REG_BIT(31)
 #define   GU_MISC_IRQ				REG_BIT(29)
 #define   DISPLAY_IRQ				REG_BIT(16)
+#define   I2C_IRQ				REG_BIT(12)
 #define   GT_DW_IRQ(x)				REG_BIT(x)
 
 /*
diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
index b0efd9b48d1e..2995d72c3f78 100644
--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -5,7 +5,7 @@
 #ifndef _XE_PMT_H_
 #define _XE_PMT_H_
 
-#define SOC_BASE			0x280000
+#include "xe_regs.h"
 
 #define BMG_PMT_BASE_OFFSET		0xDB000
 #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h b/drivers/gpu/drm/xe/regs/xe_regs.h
index 3abb17d2ca33..1926b4044314 100644
--- a/drivers/gpu/drm/xe/regs/xe_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_regs.h
@@ -7,6 +7,8 @@
 
 #include "regs/xe_reg_defs.h"
 
+#define SOC_BASE				0x280000
+
 #define GU_CNTL_PROTECTED			XE_REG(0x10100C)
 #define   DRIVERINT_FLR_DIS			REG_BIT(31)
 
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 0b73cb72bad1..6db09cfc8eb8 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -43,6 +43,7 @@
 #include "xe_guc_pc.h"
 #include "xe_hw_engine_group.h"
 #include "xe_hwmon.h"
+#include "xe_i2c.h"
 #include "xe_irq.h"
 #include "xe_mmio.h"
 #include "xe_module.h"
@@ -902,6 +903,10 @@ int xe_device_probe(struct xe_device *xe)
 	if (err)
 		goto err_unregister_display;
 
+	err = xe_i2c_probe(xe);
+	if (err)
+		goto err_unregister_display;
+
 	for_each_gt(gt, xe, id)
 		xe_gt_sanitize_freq(gt);
 
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 7e4f6d846af6..ce0c4ed834b8 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -34,6 +34,7 @@ struct dram_info;
 struct intel_display;
 struct intel_dg_nvm_dev;
 struct xe_ggtt;
+struct xe_i2c;
 struct xe_pat_ops;
 struct xe_pxp;
 
@@ -583,6 +584,9 @@ struct xe_device {
 	/** @pmu: performance monitoring unit */
 	struct xe_pmu pmu;
 
+	/** @i2c: I2C host controller */
+	struct xe_i2c *i2c;
+
 	/** @atomic_svm_timeslice_ms: Atomic SVM fault timeslice MS */
 	u32 atomic_svm_timeslice_ms;
 
diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
new file mode 100644
index 000000000000..172271469ceb
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -0,0 +1,300 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Intel Xe I2C attached Microcontroller Units (MCU)
+ *
+ * Copyright (C) 2025 Intel Corporation.
+ */
+
+#include <linux/array_size.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/notifier.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/sprintf.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "regs/xe_i2c_regs.h"
+#include "regs/xe_irq_regs.h"
+
+#include "xe_device.h"
+#include "xe_device_types.h"
+#include "xe_i2c.h"
+#include "xe_mmio.h"
+#include "xe_platform_types.h"
+
+/**
+ * DOC: Xe I2C devices
+ *
+ * Register platform device for the I2C host controller (Synpsys DesignWare I2C)
+ * if the registers of that controller are mapped to the MMIO, and also the I2C
+ * client device for the Add-In Management Controller (the MCU) attached to the
+ * host controller.
+ *
+ * See drivers/i2c/busses/i2c-designware-* for more information on the I2C host
+ * controller.
+ */
+
+static const char adapter_name[] = "i2c_designware";
+
+static const struct property_entry xe_i2c_adapter_properties[] = {
+	PROPERTY_ENTRY_STRING("compatible", "intel,xe-i2c"),
+	PROPERTY_ENTRY_U32("clock-frequency", I2C_MAX_FAST_MODE_PLUS_FREQ),
+	{ }
+};
+
+static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
+{
+	u32 *val = ep;
+
+	val[0] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_PREFIX);
+	val[1] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_POSTFIX);
+}
+
+static void xe_i2c_client_work(struct work_struct *work)
+{
+	struct xe_i2c *i2c = container_of(work, struct xe_i2c, work);
+	struct i2c_board_info info = {
+		.type	= "amc",
+		.flags	= I2C_CLIENT_HOST_NOTIFY,
+		.addr	= i2c->ep.addr[1],
+	};
+
+	i2c->client[0] = i2c_new_client_device(i2c->adapter, &info);
+}
+
+static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct xe_i2c *i2c = container_of(nb, struct xe_i2c, bus_notifier);
+	struct i2c_adapter *adapter = i2c_verify_adapter(data);
+	struct device *dev = data;
+
+	if (action == BUS_NOTIFY_ADD_DEVICE &&
+	    adapter && dev->parent == &i2c->pdev->dev) {
+		i2c->adapter = adapter;
+		schedule_work(&i2c->work);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int xe_i2c_register_adapter(struct xe_i2c *i2c)
+{
+	struct pci_dev *pci = to_pci_dev(i2c->drm_dev);
+	struct platform_device *pdev;
+	struct fwnode_handle *fwnode;
+	int ret;
+
+	fwnode = fwnode_create_software_node(xe_i2c_adapter_properties, NULL);
+	if (!fwnode)
+		return -ENOMEM;
+
+	/*
+	 * Not using platform_device_register_full() here because we don't have
+	 * a handle to the platform_device before it returns. xe_i2c_notifier()
+	 * uses that handle, but it may be called before
+	 * platform_device_register_full() is done.
+	 */
+	pdev = platform_device_alloc(adapter_name, pci_dev_id(pci));
+	if (!pdev) {
+		ret = -ENOMEM;
+		goto err_fwnode_remove;
+	}
+
+	if (i2c->adapter_irq) {
+		struct resource res;
+
+		res = DEFINE_RES_IRQ_NAMED(i2c->adapter_irq, "xe_i2c");
+
+		ret = platform_device_add_resources(pdev, &res, 1);
+		if (ret)
+			goto err_pdev_put;
+	}
+
+	pdev->dev.parent = i2c->drm_dev;
+	pdev->dev.fwnode = fwnode;
+	i2c->adapter_node = fwnode;
+	i2c->pdev = pdev;
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto err_pdev_put;
+
+	return 0;
+
+err_pdev_put:
+	platform_device_put(pdev);
+err_fwnode_remove:
+	fwnode_remove_software_node(fwnode);
+
+	return ret;
+}
+
+static void xe_i2c_unregister_adapter(struct xe_i2c *i2c)
+{
+	platform_device_unregister(i2c->pdev);
+	fwnode_remove_software_node(i2c->adapter_node);
+}
+
+/**
+ * xe_i2c_irq_handler: Handler for I2C interrupts
+ * @xe: xe device instance
+ * @master_ctl: interrupt register
+ *
+ * Forward interrupts generated by the I2C host adapter to the I2C host adapter
+ * driver.
+ */
+void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
+{
+	if (!xe->i2c || !xe->i2c->adapter_irq)
+		return;
+
+	if (master_ctl & I2C_IRQ)
+		generic_handle_irq_safe(xe->i2c->adapter_irq);
+}
+
+static int xe_i2c_irq_map(struct irq_domain *h, unsigned int virq,
+			  irq_hw_number_t hw_irq_num)
+{
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+	return 0;
+}
+
+static const struct irq_domain_ops xe_i2c_irq_ops = {
+	.map = xe_i2c_irq_map,
+};
+
+static int xe_i2c_create_irq(struct xe_i2c *i2c)
+{
+	struct irq_domain *domain;
+
+	if (!(i2c->ep.capabilities & XE_I2C_EP_CAP_IRQ))
+		return 0;
+
+	domain = irq_domain_create_linear(dev_fwnode(i2c->drm_dev), 1, &xe_i2c_irq_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
+
+	i2c->adapter_irq = irq_create_mapping(domain, 0);
+	i2c->irqdomain = domain;
+
+	return 0;
+}
+
+static void xe_i2c_remove_irq(struct xe_i2c *i2c)
+{
+	if (!i2c->irqdomain)
+		return;
+
+	irq_dispose_mapping(i2c->adapter_irq);
+	irq_domain_remove(i2c->irqdomain);
+}
+
+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));
+
+	return 0;
+}
+
+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);
+
+	return 0;
+}
+
+static const struct regmap_config i2c_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_read = xe_i2c_read,
+	.reg_write = xe_i2c_write,
+	.fast_io = true,
+};
+
+static void xe_i2c_remove(void *data)
+{
+	struct xe_i2c *i2c = data;
+	unsigned int i;
+
+	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
+		i2c_unregister_device(i2c->client[i]);
+
+	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
+	xe_i2c_unregister_adapter(i2c);
+	xe_i2c_remove_irq(i2c);
+}
+
+/**
+ * xe_i2c_probe: Probe the I2C host adapter and the I2C clients attached to it
+ * @xe: xe device instance
+ *
+ * Register all the I2C devices described in the I2C Endpoint data structure.
+ *
+ * Return: 0 on success, error code on failure
+ */
+int xe_i2c_probe(struct xe_device *xe)
+{
+	struct device *drm_dev = xe->drm.dev;
+	struct xe_i2c_endpoint ep;
+	struct regmap *regmap;
+	struct xe_i2c *i2c;
+	int ret;
+
+	if (xe->info.platform != XE_BATTLEMAGE)
+		return 0;
+
+	xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
+	if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
+		return 0;
+
+	i2c = devm_kzalloc(drm_dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	INIT_WORK(&i2c->work, xe_i2c_client_work);
+	i2c->mmio = xe_root_tile_mmio(xe);
+	i2c->drm_dev = drm_dev;
+	i2c->ep = ep;
+
+	regmap = devm_regmap_init(drm_dev, NULL, i2c, &i2c_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	i2c->bus_notifier.notifier_call = xe_i2c_notifier;
+	ret = bus_register_notifier(&i2c_bus_type, &i2c->bus_notifier);
+	if (ret)
+		return ret;
+
+	ret = xe_i2c_create_irq(i2c);
+	if (ret)
+		goto err_unregister_notifier;
+
+	ret = xe_i2c_register_adapter(i2c);
+	if (ret)
+		goto err_remove_irq;
+
+	return devm_add_action_or_reset(drm_dev, xe_i2c_remove, i2c);
+
+err_remove_irq:
+	xe_i2c_remove_irq(i2c);
+
+err_unregister_notifier:
+	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
new file mode 100644
index 000000000000..7ea40f4e4aa4
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_i2c.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: MIT */
+#ifndef _XE_I2C_H_
+#define _XE_I2C_H_
+
+#include <linux/bits.h>
+#include <linux/notifier.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+struct device;
+struct fwnode_handle;
+struct i2c_adapter;
+struct i2c_client;
+struct irq_domain;
+struct platform_device;
+struct xe_device;
+struct xe_mmio;
+
+#define XE_I2C_MAX_CLIENTS		3
+
+#define XE_I2C_EP_COOKIE_DEVICE		0xde
+
+/* Endpoint Capabilities */
+#define XE_I2C_EP_CAP_IRQ		BIT(0)
+
+struct xe_i2c_endpoint {
+	u8 cookie;
+	u8 capabilities;
+	u16 addr[XE_I2C_MAX_CLIENTS];
+};
+
+struct xe_i2c {
+	struct fwnode_handle *adapter_node;
+	struct platform_device *pdev;
+	struct i2c_adapter *adapter;
+	struct i2c_client *client[XE_I2C_MAX_CLIENTS];
+
+	struct notifier_block bus_notifier;
+	struct work_struct work;
+
+	struct irq_domain *irqdomain;
+	int adapter_irq;
+
+	struct xe_i2c_endpoint ep;
+	struct device *drm_dev;
+
+	struct xe_mmio *mmio;
+};
+
+#if IS_ENABLED(CONFIG_I2C)
+int xe_i2c_probe(struct xe_device *xe);
+void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl);
+#else
+static inline int xe_i2c_probe(struct xe_device *xe) { return 0; }
+static inline void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl) { }
+#endif
+
+#endif
diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
index 5362d3174b06..c43e62dc692e 100644
--- a/drivers/gpu/drm/xe/xe_irq.c
+++ b/drivers/gpu/drm/xe/xe_irq.c
@@ -18,6 +18,7 @@
 #include "xe_gt.h"
 #include "xe_guc.h"
 #include "xe_hw_engine.h"
+#include "xe_i2c.h"
 #include "xe_memirq.h"
 #include "xe_mmio.h"
 #include "xe_pxp.h"
@@ -476,6 +477,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
 			if (xe->info.has_heci_cscfi)
 				xe_heci_csc_irq_handler(xe, master_ctl);
 			xe_display_irq_handler(xe, master_ctl);
+			xe_i2c_irq_handler(xe, master_ctl);
 			gu_misc_iir = gu_misc_irq_ack(xe, master_ctl);
 		}
 	}
-- 
2.47.2

[PATCH v5 3/4] drm/xe/pm: Wire up suspend/resume for I2C controller

[Heikki Krogerus]

From: Raag Jadav <raag.jadav@intel.com>

Wire up suspend/resume handles for I2C controller to match its power
state with SGUnit.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/gpu/drm/xe/regs/xe_i2c_regs.h |  5 +++++
 drivers/gpu/drm/xe/xe_i2c.c           | 29 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_i2c.h           |  4 ++++
 drivers/gpu/drm/xe/xe_pm.c            |  9 +++++++++
 4 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
index 92dae4487614..af781c8e4a80 100644
--- a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
@@ -2,6 +2,8 @@
 #ifndef _XE_I2C_REGS_H_
 #define _XE_I2C_REGS_H_
 
+#include <linux/pci_regs.h>
+
 #include "xe_reg_defs.h"
 #include "xe_regs.h"
 
@@ -12,4 +14,7 @@
 #define REG_SG_REMAP_ADDR_PREFIX	XE_REG(SOC_BASE + 0x0164)
 #define REG_SG_REMAP_ADDR_POSTFIX	XE_REG(SOC_BASE + 0x0168)
 
+#define I2C_CONFIG_CMD			XE_REG(I2C_CONFIG_SPACE_OFFSET + PCI_COMMAND)
+#define I2C_CONFIG_PMCSR		XE_REG(I2C_CONFIG_SPACE_OFFSET + 0x84)
+
 #endif /* _XE_I2C_REGS_H_ */
diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
index 172271469ceb..c22800256dcf 100644
--- a/drivers/gpu/drm/xe/xe_i2c.c
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -226,6 +226,31 @@ static const struct regmap_config i2c_regmap_config = {
 	.fast_io = true,
 };
 
+void xe_i2c_pm_suspend(struct xe_device *xe)
+{
+	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
+
+	if (!xe->i2c || xe->i2c->ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
+		return;
+
+	xe_mmio_rmw32(mmio, I2C_CONFIG_PMCSR, PCI_PM_CTRL_STATE_MASK, (__force u32)PCI_D3hot);
+	drm_dbg(&xe->drm, "pmcsr: 0x%08x\n", xe_mmio_read32(mmio, I2C_CONFIG_PMCSR));
+}
+
+void xe_i2c_pm_resume(struct xe_device *xe, bool d3cold)
+{
+	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
+
+	if (!xe->i2c || xe->i2c->ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
+		return;
+
+	if (d3cold)
+		xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_MEMORY);
+
+	xe_mmio_rmw32(mmio, I2C_CONFIG_PMCSR, PCI_PM_CTRL_STATE_MASK, (__force u32)PCI_D0);
+	drm_dbg(&xe->drm, "pmcsr: 0x%08x\n", xe_mmio_read32(mmio, I2C_CONFIG_PMCSR));
+}
+
 static void xe_i2c_remove(void *data)
 {
 	struct xe_i2c *i2c = data;
@@ -270,6 +295,10 @@ int xe_i2c_probe(struct xe_device *xe)
 	i2c->mmio = xe_root_tile_mmio(xe);
 	i2c->drm_dev = drm_dev;
 	i2c->ep = ep;
+	xe->i2c = i2c;
+
+	/* PCI PM isn't aware of this device, bring it up and match it with SGUnit state. */
+	xe_i2c_pm_resume(xe, true);
 
 	regmap = devm_regmap_init(drm_dev, NULL, i2c, &i2c_regmap_config);
 	if (IS_ERR(regmap))
diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
index 7ea40f4e4aa4..b767ed8ce52b 100644
--- a/drivers/gpu/drm/xe/xe_i2c.h
+++ b/drivers/gpu/drm/xe/xe_i2c.h
@@ -50,9 +50,13 @@ struct xe_i2c {
 #if IS_ENABLED(CONFIG_I2C)
 int xe_i2c_probe(struct xe_device *xe);
 void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl);
+void xe_i2c_pm_suspend(struct xe_device *xe);
+void xe_i2c_pm_resume(struct xe_device *xe, bool d3cold);
 #else
 static inline int xe_i2c_probe(struct xe_device *xe) { return 0; }
 static inline void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl) { }
+static inline void xe_i2c_pm_suspend(struct xe_device *xe) { }
+static inline void xe_i2c_pm_resume(struct xe_device *xe, bool d3cold) { }
 #endif
 
 #endif
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 26e95460af87..46471e166b96 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -19,6 +19,7 @@
 #include "xe_ggtt.h"
 #include "xe_gt.h"
 #include "xe_guc.h"
+#include "xe_i2c.h"
 #include "xe_irq.h"
 #include "xe_pcode.h"
 #include "xe_pxp.h"
@@ -146,6 +147,8 @@ int xe_pm_suspend(struct xe_device *xe)
 
 	xe_display_pm_suspend_late(xe);
 
+	xe_i2c_pm_suspend(xe);
+
 	drm_dbg(&xe->drm, "Device suspended\n");
 	return 0;
 
@@ -191,6 +194,8 @@ int xe_pm_resume(struct xe_device *xe)
 	if (err)
 		goto err;
 
+	xe_i2c_pm_resume(xe, xe->d3cold.allowed);
+
 	xe_irq_resume(xe);
 
 	for_each_gt(gt, xe, id)
@@ -484,6 +489,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
 
 	xe_display_pm_runtime_suspend_late(xe);
 
+	xe_i2c_pm_suspend(xe);
+
 	xe_rpm_lockmap_release(xe);
 	xe_pm_write_callback_task(xe, NULL);
 	return 0;
@@ -531,6 +538,8 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 			goto out;
 	}
 
+	xe_i2c_pm_resume(xe, xe->d3cold.allowed);
+
 	xe_irq_resume(xe);
 
 	for_each_gt(gt, xe, id)
-- 
2.47.2

[PATCH v5 4/4] drm/xe/xe_i2c: Add support for i2c in survivability mode

[Heikki Krogerus]

From: Riana Tauro <riana.tauro@intel.com>

Initialize i2c in survivability mode to allow firmware
update of Add-In Management Controller (AMC) in
survivability mode.

Signed-off-by: Riana Tauro <riana.tauro@intel.com>
Reviewed-by: Raag Jadav <raag.jadav@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_survivability_mode.c | 23 ++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/drm/xe/xe_survivability_mode.c
index 1f710b3fc599..76141eb2c853 100644
--- a/drivers/gpu/drm/xe/xe_survivability_mode.c
+++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
@@ -14,6 +14,7 @@
 #include "xe_device.h"
 #include "xe_gt.h"
 #include "xe_heci_gsc.h"
+#include "xe_i2c.h"
 #include "xe_mmio.h"
 #include "xe_pcode_api.h"
 #include "xe_vsec.h"
@@ -173,20 +174,26 @@ static int enable_survivability_mode(struct pci_dev *pdev)
 	survivability->mode = true;
 
 	ret = xe_heci_gsc_init(xe);
-	if (ret) {
-		/*
-		 * But if it fails, device can't enter survivability
-		 * so move it back for correct error handling
-		 */
-		survivability->mode = false;
-		return ret;
-	}
+	if (ret)
+		goto err;
 
 	xe_vsec_init(xe);
 
+	ret = xe_i2c_probe(xe);
+	if (ret)
+		goto err;
+
 	dev_err(dev, "In Survivability Mode\n");
 
 	return 0;
+
+err:
+	/*
+	 * But if it fails, device can't enter survivability
+	 * so move it back for correct error handling.
+	 */
+	survivability->mode = false;
+	return ret;
 }
 
 /**
-- 
2.47.2

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andy Shevchenko]

On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> The regmap is coming from the parent also in case of Xe
> GPUs. Reusing the Wangxun quirk for that.

...

>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
> +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);

> -	dev->flags = (uintptr_t)device_get_match_data(device);
>  	if (device_property_present(device, "wx,i2c-snps-model"))
> -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
>  
>  	dev->dev = device;
>  	dev->irq = irq;
> +	dev->flags = flags;

Maybe I'm missing something, but why do we need these (above) changes?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/4] drm/xe: Support for I2C attached MCUs

[Rodrigo Vivi]

On Fri, Jun 27, 2025 at 04:53:12PM +0300, Heikki Krogerus wrote:
> Adding adaption/glue layer where the I2C host adapter
> (Synopsys DesignWare I2C adapter) and the I2C clients (the
> microcontroller units) are enumerated.
> 
> The microcontroller units (MCU) that are attached to the GPU
> depend on the OEM. The initially supported MCU will be the
> Add-In Management Controller (AMC).
> 

I hope all Andy's comments got addressed here..

Mine's were. Thank you.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/Kconfig            |   1 +
>  drivers/gpu/drm/xe/Makefile           |   1 +
>  drivers/gpu/drm/xe/regs/xe_i2c_regs.h |  15 ++
>  drivers/gpu/drm/xe/regs/xe_irq_regs.h |   1 +
>  drivers/gpu/drm/xe/regs/xe_pmt.h      |   2 +-
>  drivers/gpu/drm/xe/regs/xe_regs.h     |   2 +
>  drivers/gpu/drm/xe/xe_device.c        |   5 +
>  drivers/gpu/drm/xe/xe_device_types.h  |   4 +
>  drivers/gpu/drm/xe/xe_i2c.c           | 300 ++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_i2c.h           |  58 +++++
>  drivers/gpu/drm/xe/xe_irq.c           |   2 +
>  11 files changed, 390 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/xe/regs/xe_i2c_regs.h
>  create mode 100644 drivers/gpu/drm/xe/xe_i2c.c
>  create mode 100644 drivers/gpu/drm/xe/xe_i2c.h
> 
> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> index 553c29e1030b..eb361c202581 100644
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -44,6 +44,7 @@ config DRM_XE
>  	select WANT_DEV_COREDUMP
>  	select AUXILIARY_BUS
>  	select HMM_MIRROR
> +	select REGMAP if I2C
>  	help
>  	  Driver for Intel Xe2 series GPUs and later. Experimental support
>  	  for Xe series is also available.
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index eee6bac01a00..4b8a8fcd3959 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -125,6 +125,7 @@ xe-y += xe_bb.o \
>  	xe_wait_user_fence.o \
>  	xe_wopcm.o
>  
> +xe-$(CONFIG_I2C)	+= xe_i2c.o
>  xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
>  xe-$(CONFIG_DRM_XE_GPUSVM) += xe_svm.o
>  
> diff --git a/drivers/gpu/drm/xe/regs/xe_i2c_regs.h b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
> new file mode 100644
> index 000000000000..92dae4487614
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/regs/xe_i2c_regs.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +#ifndef _XE_I2C_REGS_H_
> +#define _XE_I2C_REGS_H_
> +
> +#include "xe_reg_defs.h"
> +#include "xe_regs.h"
> +
> +#define I2C_BRIDGE_OFFSET		(SOC_BASE + 0xd9000)
> +#define I2C_CONFIG_SPACE_OFFSET		(SOC_BASE + 0xf6000)
> +#define I2C_MEM_SPACE_OFFSET		(SOC_BASE + 0xf7400)
> +
> +#define REG_SG_REMAP_ADDR_PREFIX	XE_REG(SOC_BASE + 0x0164)
> +#define REG_SG_REMAP_ADDR_POSTFIX	XE_REG(SOC_BASE + 0x0168)
> +
> +#endif /* _XE_I2C_REGS_H_ */
> diff --git a/drivers/gpu/drm/xe/regs/xe_irq_regs.h b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> index f0ecfcac4003..13635e4331d4 100644
> --- a/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> @@ -19,6 +19,7 @@
>  #define   MASTER_IRQ				REG_BIT(31)
>  #define   GU_MISC_IRQ				REG_BIT(29)
>  #define   DISPLAY_IRQ				REG_BIT(16)
> +#define   I2C_IRQ				REG_BIT(12)
>  #define   GT_DW_IRQ(x)				REG_BIT(x)
>  
>  /*
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index b0efd9b48d1e..2995d72c3f78 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -5,7 +5,7 @@
>  #ifndef _XE_PMT_H_
>  #define _XE_PMT_H_
>  
> -#define SOC_BASE			0x280000
> +#include "xe_regs.h"
>  
>  #define BMG_PMT_BASE_OFFSET		0xDB000
>  #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
> diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h b/drivers/gpu/drm/xe/regs/xe_regs.h
> index 3abb17d2ca33..1926b4044314 100644
> --- a/drivers/gpu/drm/xe/regs/xe_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_regs.h
> @@ -7,6 +7,8 @@
>  
>  #include "regs/xe_reg_defs.h"
>  
> +#define SOC_BASE				0x280000
> +
>  #define GU_CNTL_PROTECTED			XE_REG(0x10100C)
>  #define   DRIVERINT_FLR_DIS			REG_BIT(31)
>  
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 0b73cb72bad1..6db09cfc8eb8 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -43,6 +43,7 @@
>  #include "xe_guc_pc.h"
>  #include "xe_hw_engine_group.h"
>  #include "xe_hwmon.h"
> +#include "xe_i2c.h"
>  #include "xe_irq.h"
>  #include "xe_mmio.h"
>  #include "xe_module.h"
> @@ -902,6 +903,10 @@ int xe_device_probe(struct xe_device *xe)
>  	if (err)
>  		goto err_unregister_display;
>  
> +	err = xe_i2c_probe(xe);
> +	if (err)
> +		goto err_unregister_display;
> +
>  	for_each_gt(gt, xe, id)
>  		xe_gt_sanitize_freq(gt);
>  
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 7e4f6d846af6..ce0c4ed834b8 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -34,6 +34,7 @@ struct dram_info;
>  struct intel_display;
>  struct intel_dg_nvm_dev;
>  struct xe_ggtt;
> +struct xe_i2c;
>  struct xe_pat_ops;
>  struct xe_pxp;
>  
> @@ -583,6 +584,9 @@ struct xe_device {
>  	/** @pmu: performance monitoring unit */
>  	struct xe_pmu pmu;
>  
> +	/** @i2c: I2C host controller */
> +	struct xe_i2c *i2c;
> +
>  	/** @atomic_svm_timeslice_ms: Atomic SVM fault timeslice MS */
>  	u32 atomic_svm_timeslice_ms;
>  
> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> new file mode 100644
> index 000000000000..172271469ceb
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
> @@ -0,0 +1,300 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Intel Xe I2C attached Microcontroller Units (MCU)
> + *
> + * Copyright (C) 2025 Intel Corporation.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/notifier.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/sprintf.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +#include "regs/xe_i2c_regs.h"
> +#include "regs/xe_irq_regs.h"
> +
> +#include "xe_device.h"
> +#include "xe_device_types.h"
> +#include "xe_i2c.h"
> +#include "xe_mmio.h"
> +#include "xe_platform_types.h"
> +
> +/**
> + * DOC: Xe I2C devices
> + *
> + * Register platform device for the I2C host controller (Synpsys DesignWare I2C)
> + * if the registers of that controller are mapped to the MMIO, and also the I2C
> + * client device for the Add-In Management Controller (the MCU) attached to the
> + * host controller.
> + *
> + * See drivers/i2c/busses/i2c-designware-* for more information on the I2C host
> + * controller.
> + */
> +
> +static const char adapter_name[] = "i2c_designware";
> +
> +static const struct property_entry xe_i2c_adapter_properties[] = {
> +	PROPERTY_ENTRY_STRING("compatible", "intel,xe-i2c"),
> +	PROPERTY_ENTRY_U32("clock-frequency", I2C_MAX_FAST_MODE_PLUS_FREQ),
> +	{ }
> +};
> +
> +static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
> +{
> +	u32 *val = ep;
> +
> +	val[0] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_PREFIX);
> +	val[1] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_POSTFIX);
> +}
> +
> +static void xe_i2c_client_work(struct work_struct *work)
> +{
> +	struct xe_i2c *i2c = container_of(work, struct xe_i2c, work);
> +	struct i2c_board_info info = {
> +		.type	= "amc",
> +		.flags	= I2C_CLIENT_HOST_NOTIFY,
> +		.addr	= i2c->ep.addr[1],
> +	};
> +
> +	i2c->client[0] = i2c_new_client_device(i2c->adapter, &info);
> +}
> +
> +static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct xe_i2c *i2c = container_of(nb, struct xe_i2c, bus_notifier);
> +	struct i2c_adapter *adapter = i2c_verify_adapter(data);
> +	struct device *dev = data;
> +
> +	if (action == BUS_NOTIFY_ADD_DEVICE &&
> +	    adapter && dev->parent == &i2c->pdev->dev) {
> +		i2c->adapter = adapter;
> +		schedule_work(&i2c->work);
> +		return NOTIFY_OK;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int xe_i2c_register_adapter(struct xe_i2c *i2c)
> +{
> +	struct pci_dev *pci = to_pci_dev(i2c->drm_dev);
> +	struct platform_device *pdev;
> +	struct fwnode_handle *fwnode;
> +	int ret;
> +
> +	fwnode = fwnode_create_software_node(xe_i2c_adapter_properties, NULL);
> +	if (!fwnode)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Not using platform_device_register_full() here because we don't have
> +	 * a handle to the platform_device before it returns. xe_i2c_notifier()
> +	 * uses that handle, but it may be called before
> +	 * platform_device_register_full() is done.
> +	 */
> +	pdev = platform_device_alloc(adapter_name, pci_dev_id(pci));
> +	if (!pdev) {
> +		ret = -ENOMEM;
> +		goto err_fwnode_remove;
> +	}
> +
> +	if (i2c->adapter_irq) {
> +		struct resource res;
> +
> +		res = DEFINE_RES_IRQ_NAMED(i2c->adapter_irq, "xe_i2c");
> +
> +		ret = platform_device_add_resources(pdev, &res, 1);
> +		if (ret)
> +			goto err_pdev_put;
> +	}
> +
> +	pdev->dev.parent = i2c->drm_dev;
> +	pdev->dev.fwnode = fwnode;
> +	i2c->adapter_node = fwnode;
> +	i2c->pdev = pdev;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto err_pdev_put;
> +
> +	return 0;
> +
> +err_pdev_put:
> +	platform_device_put(pdev);
> +err_fwnode_remove:
> +	fwnode_remove_software_node(fwnode);
> +
> +	return ret;
> +}
> +
> +static void xe_i2c_unregister_adapter(struct xe_i2c *i2c)
> +{
> +	platform_device_unregister(i2c->pdev);
> +	fwnode_remove_software_node(i2c->adapter_node);
> +}
> +
> +/**
> + * xe_i2c_irq_handler: Handler for I2C interrupts
> + * @xe: xe device instance
> + * @master_ctl: interrupt register
> + *
> + * Forward interrupts generated by the I2C host adapter to the I2C host adapter
> + * driver.
> + */
> +void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
> +{
> +	if (!xe->i2c || !xe->i2c->adapter_irq)
> +		return;
> +
> +	if (master_ctl & I2C_IRQ)
> +		generic_handle_irq_safe(xe->i2c->adapter_irq);
> +}
> +
> +static int xe_i2c_irq_map(struct irq_domain *h, unsigned int virq,
> +			  irq_hw_number_t hw_irq_num)
> +{
> +	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops xe_i2c_irq_ops = {
> +	.map = xe_i2c_irq_map,
> +};
> +
> +static int xe_i2c_create_irq(struct xe_i2c *i2c)
> +{
> +	struct irq_domain *domain;
> +
> +	if (!(i2c->ep.capabilities & XE_I2C_EP_CAP_IRQ))
> +		return 0;
> +
> +	domain = irq_domain_create_linear(dev_fwnode(i2c->drm_dev), 1, &xe_i2c_irq_ops, NULL);
> +	if (!domain)
> +		return -ENOMEM;
> +
> +	i2c->adapter_irq = irq_create_mapping(domain, 0);
> +	i2c->irqdomain = domain;
> +
> +	return 0;
> +}
> +
> +static void xe_i2c_remove_irq(struct xe_i2c *i2c)
> +{
> +	if (!i2c->irqdomain)
> +		return;
> +
> +	irq_dispose_mapping(i2c->adapter_irq);
> +	irq_domain_remove(i2c->irqdomain);
> +}
> +
> +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));
> +
> +	return 0;
> +}
> +
> +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);
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config i2c_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_read = xe_i2c_read,
> +	.reg_write = xe_i2c_write,
> +	.fast_io = true,
> +};
> +
> +static void xe_i2c_remove(void *data)
> +{
> +	struct xe_i2c *i2c = data;
> +	unsigned int i;
> +
> +	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> +		i2c_unregister_device(i2c->client[i]);
> +
> +	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> +	xe_i2c_unregister_adapter(i2c);
> +	xe_i2c_remove_irq(i2c);
> +}
> +
> +/**
> + * xe_i2c_probe: Probe the I2C host adapter and the I2C clients attached to it
> + * @xe: xe device instance
> + *
> + * Register all the I2C devices described in the I2C Endpoint data structure.
> + *
> + * Return: 0 on success, error code on failure
> + */
> +int xe_i2c_probe(struct xe_device *xe)
> +{
> +	struct device *drm_dev = xe->drm.dev;
> +	struct xe_i2c_endpoint ep;
> +	struct regmap *regmap;
> +	struct xe_i2c *i2c;
> +	int ret;
> +
> +	if (xe->info.platform != XE_BATTLEMAGE)
> +		return 0;
> +
> +	xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
> +	if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
> +		return 0;
> +
> +	i2c = devm_kzalloc(drm_dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&i2c->work, xe_i2c_client_work);
> +	i2c->mmio = xe_root_tile_mmio(xe);
> +	i2c->drm_dev = drm_dev;
> +	i2c->ep = ep;
> +
> +	regmap = devm_regmap_init(drm_dev, NULL, i2c, &i2c_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	i2c->bus_notifier.notifier_call = xe_i2c_notifier;
> +	ret = bus_register_notifier(&i2c_bus_type, &i2c->bus_notifier);
> +	if (ret)
> +		return ret;
> +
> +	ret = xe_i2c_create_irq(i2c);
> +	if (ret)
> +		goto err_unregister_notifier;
> +
> +	ret = xe_i2c_register_adapter(i2c);
> +	if (ret)
> +		goto err_remove_irq;
> +
> +	return devm_add_action_or_reset(drm_dev, xe_i2c_remove, i2c);
> +
> +err_remove_irq:
> +	xe_i2c_remove_irq(i2c);
> +
> +err_unregister_notifier:
> +	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
> new file mode 100644
> index 000000000000..7ea40f4e4aa4
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_i2c.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: MIT */
> +#ifndef _XE_I2C_H_
> +#define _XE_I2C_H_
> +
> +#include <linux/bits.h>
> +#include <linux/notifier.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +struct device;
> +struct fwnode_handle;
> +struct i2c_adapter;
> +struct i2c_client;
> +struct irq_domain;
> +struct platform_device;
> +struct xe_device;
> +struct xe_mmio;
> +
> +#define XE_I2C_MAX_CLIENTS		3
> +
> +#define XE_I2C_EP_COOKIE_DEVICE		0xde
> +
> +/* Endpoint Capabilities */
> +#define XE_I2C_EP_CAP_IRQ		BIT(0)
> +
> +struct xe_i2c_endpoint {
> +	u8 cookie;
> +	u8 capabilities;
> +	u16 addr[XE_I2C_MAX_CLIENTS];
> +};
> +
> +struct xe_i2c {
> +	struct fwnode_handle *adapter_node;
> +	struct platform_device *pdev;
> +	struct i2c_adapter *adapter;
> +	struct i2c_client *client[XE_I2C_MAX_CLIENTS];
> +
> +	struct notifier_block bus_notifier;
> +	struct work_struct work;
> +
> +	struct irq_domain *irqdomain;
> +	int adapter_irq;
> +
> +	struct xe_i2c_endpoint ep;
> +	struct device *drm_dev;
> +
> +	struct xe_mmio *mmio;
> +};
> +
> +#if IS_ENABLED(CONFIG_I2C)
> +int xe_i2c_probe(struct xe_device *xe);
> +void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl);
> +#else
> +static inline int xe_i2c_probe(struct xe_device *xe) { return 0; }
> +static inline void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl) { }
> +#endif
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 5362d3174b06..c43e62dc692e 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -18,6 +18,7 @@
>  #include "xe_gt.h"
>  #include "xe_guc.h"
>  #include "xe_hw_engine.h"
> +#include "xe_i2c.h"
>  #include "xe_memirq.h"
>  #include "xe_mmio.h"
>  #include "xe_pxp.h"
> @@ -476,6 +477,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>  			if (xe->info.has_heci_cscfi)
>  				xe_heci_csc_irq_handler(xe, master_ctl);
>  			xe_display_irq_handler(xe, master_ctl);
> +			xe_i2c_irq_handler(xe, master_ctl);
>  			gu_misc_iir = gu_misc_irq_ack(xe, master_ctl);
>  		}
>  	}
> -- 
> 2.47.2
> 

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Rodrigo Vivi]

On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> > The regmap is coming from the parent also in case of Xe
> > GPUs. Reusing the Wangxun quirk for that.
> 
> ...
> 
> >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> >  {
> > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> 
> > -	dev->flags = (uintptr_t)device_get_match_data(device);
> >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> >  
> >  	dev->dev = device;
> >  	dev->irq = irq;
> > +	dev->flags = flags;
> 
> Maybe I'm missing something, but why do we need these (above) changes?


in between, it is introduced a new one:
flags |= ACCESS_POLLING;

So, the initialization moved up, before the ACCESS_POLLING, and
it let the assignment to the last, along with the group.

Looks good to me:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andy Shevchenko]

On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:

...

> > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > >  {
> > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > 
> > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > >  
> > >  	dev->dev = device;
> > >  	dev->irq = irq;
> > > +	dev->flags = flags;
> > 
> > Maybe I'm missing something, but why do we need these (above) changes?
> 
> in between, it is introduced a new one:
> flags |= ACCESS_POLLING;
> 
> So, the initialization moved up, before the ACCESS_POLLING, and
> it let the assignment to the last, along with the group.

I still don't get. The cited code is complete equivalent.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Heikki Krogerus]

On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > >  {
> > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > 
> > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > >  
> > > >  	dev->dev = device;
> > > >  	dev->irq = irq;
> > > > +	dev->flags = flags;
> > > 
> > > Maybe I'm missing something, but why do we need these (above) changes?
> > 
> > in between, it is introduced a new one:
> > flags |= ACCESS_POLLING;
> > 
> > So, the initialization moved up, before the ACCESS_POLLING, and
> > it let the assignment to the last, along with the group.
> 
> I still don't get. The cited code is complete equivalent.

This was requested by Jarkko.

thanks,

-- 
heikki

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:

...

> > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > >  {
> > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > 
> > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > >  
> > > > >  	dev->dev = device;
> > > > >  	dev->irq = irq;
> > > > > +	dev->flags = flags;
> > > > 
> > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > 
> > > in between, it is introduced a new one:
> > > flags |= ACCESS_POLLING;
> > > 
> > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > it let the assignment to the last, along with the group.
> > 
> > I still don't get. The cited code is complete equivalent.
> 
> This was requested by Jarkko.

Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
required?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Heikki Krogerus]

On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > >  {
> > > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > 
> > > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > >  
> > > > > >  	dev->dev = device;
> > > > > >  	dev->irq = irq;
> > > > > > +	dev->flags = flags;
> > > > > 
> > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > 
> > > > in between, it is introduced a new one:
> > > > flags |= ACCESS_POLLING;
> > > > 
> > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > it let the assignment to the last, along with the group.
> > > 
> > > I still don't get. The cited code is complete equivalent.
> > 
> > This was requested by Jarkko.
> 
> Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> required?

You need to ask why from Jarkko - I did not really question him on
this one. Unfortunately his on vacation at the moment. I can drop
this, but then I'll have to drop also Jarkko's ACK.

I think we already agreed that this function, and probable the entire
file, need to be refactored a bit, so would you mind much if we just
went ahead with this as it is?

I'm asking that also because I don't have means or time to test this
anymore before I start my vacation.

thanks,

-- 
heikki

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:

...

> > > > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > >  {
> > > > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > 
> > > > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > >  
> > > > > > >  	dev->dev = device;
> > > > > > >  	dev->irq = irq;
> > > > > > > +	dev->flags = flags;
> > > > > > 
> > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > 
> > > > > in between, it is introduced a new one:
> > > > > flags |= ACCESS_POLLING;
> > > > > 
> > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > it let the assignment to the last, along with the group.
> > > > 
> > > > I still don't get. The cited code is complete equivalent.
> > > 
> > > This was requested by Jarkko.
> > 
> > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > required?
> 
> You need to ask why from Jarkko - I did not really question him on
> this one. Unfortunately his on vacation at the moment.

Yeah :-(

> I can drop this, but then I'll have to drop also Jarkko's ACK.

I can give mine if it helps. The code as far as I can see is 100% equivalent.

> I think we already agreed that this function, and probable the entire
> file, need to be refactored a bit, so would you mind much if we just
> went ahead with this as it is?
> 
> I'm asking that also because I don't have means or time to test this
> anymore before I start my vacation.

I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
no additional churn added without a good reason.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Heikki Krogerus]

On Mon, Jun 30, 2025 at 04:16:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > > > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > > 
> > > > > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > >  
> > > > > > > >  	dev->dev = device;
> > > > > > > >  	dev->irq = irq;
> > > > > > > > +	dev->flags = flags;
> > > > > > > 
> > > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > > 
> > > > > > in between, it is introduced a new one:
> > > > > > flags |= ACCESS_POLLING;
> > > > > > 
> > > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > > it let the assignment to the last, along with the group.
> > > > > 
> > > > > I still don't get. The cited code is complete equivalent.
> > > > 
> > > > This was requested by Jarkko.
> > > 
> > > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > > required?
> > 
> > You need to ask why from Jarkko - I did not really question him on
> > this one. Unfortunately his on vacation at the moment.
> 
> Yeah :-(
> 
> > I can drop this, but then I'll have to drop also Jarkko's ACK.
> 
> I can give mine if it helps. The code as far as I can see is 100% equivalent.
> 
> > I think we already agreed that this function, and probable the entire
> > file, need to be refactored a bit, so would you mind much if we just
> > went ahead with this as it is?
> > 
> > I'm asking that also because I don't have means or time to test this
> > anymore before I start my vacation.
> 
> I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
> no additional churn added without a good reason.

OK. I'll fix this.

thanks,

-- 
heikki

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andi Shyti]

Hi Andy,

On Mon, Jun 30, 2025 at 04:16:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > > > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > > 
> > > > > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > >  
> > > > > > > >  	dev->dev = device;
> > > > > > > >  	dev->irq = irq;
> > > > > > > > +	dev->flags = flags;
> > > > > > > 
> > > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > > 
> > > > > > in between, it is introduced a new one:
> > > > > > flags |= ACCESS_POLLING;
> > > > > > 
> > > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > > it let the assignment to the last, along with the group.
> > > > > 
> > > > > I still don't get. The cited code is complete equivalent.
> > > > 
> > > > This was requested by Jarkko.
> > > 
> > > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > > required?

I don't think it makes sense to add the extra flag later as it
would be a non relevant change, so that I'm fine with having it
here.

> > You need to ask why from Jarkko - I did not really question him on
> > this one. Unfortunately his on vacation at the moment.
> 
> Yeah :-(
> 
> > I can drop this, but then I'll have to drop also Jarkko's ACK.

Just for reference, Ack is not Reviewed. If Jarkko Acks, I assume
he is OK with the general idea, not specifically with the code. I
think that if you change a bit the code without altering the
result you can keep the Ack. Up to you.

> I can give mine if it helps. The code as far as I can see is 100% equivalent.
> 
> > I think we already agreed that this function, and probable the entire
> > file, need to be refactored a bit, so would you mind much if we just
> > went ahead with this as it is?
> > 
> > I'm asking that also because I don't have means or time to test this
> > anymore before I start my vacation.
> 
> I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
> no additional churn added without a good reason.

To be honest I don't really mind. I think that if we add the
extra "flag" or we don't, the amount of code is the same.

If we decide not to add the extra "flag", then we need to move
the initialization of dev->flag above or shuffle something else
around.

In the meantime:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Andi Shyti]

Hi Heikki,

On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> The regmap is coming from the parent also in case of Xe
> GPUs. Reusing the Wangxun quirk for that.

What I don't like, though, is that there is no mention of the
change in the probe in the commit log.

Besides, are these changes related? Can we have them split in two
different patches? I guess this would also make Andy's point
about the extra churn.

Andi

> Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v5 2/4] drm/xe: Support for I2C attached MCUs

[Andi Shyti]

Hi,

On Fri, Jun 27, 2025 at 04:53:12PM +0300, Heikki Krogerus wrote:
> Adding adaption/glue layer where the I2C host adapter
> (Synopsys DesignWare I2C adapter) and the I2C clients (the
> microcontroller units) are enumerated.
> 
> The microcontroller units (MCU) that are attached to the GPU
> depend on the OEM. The initially supported MCU will be the
> Add-In Management Controller (AMC).
> 
> Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Looks very good.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

(sorry for coming this late, but I've been on and off in this
period. I would have definitely loved to help on this)

Thanks,
Andi

Re: [PATCH v5 3/4] drm/xe/pm: Wire up suspend/resume for I2C controller

[Andi Shyti]

Hi Heikki and Raag,

On Fri, Jun 27, 2025 at 04:53:13PM +0300, Heikki Krogerus wrote:
> From: Raag Jadav <raag.jadav@intel.com>
> 
> Wire up suspend/resume handles for I2C controller to match its power
> state with SGUnit.
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH v5 4/4] drm/xe/xe_i2c: Add support for i2c in survivability mode

[Andi Shyti]

Hi Heikki and Riana,

> +err:
> +	/*
> +	 * But if it fails, device can't enter survivability

nitpick: the "But" follows the "if". As we don't have the "if"
anymore, the "But" leaves the reader a bit flubbergasted :-)

If you are going to resend, please, remove it. Otherwise it can
be improved before submitting the patch.

In any case,

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Andi

Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

[Heikki Krogerus]

On Mon, Jun 30, 2025 at 08:28:12PM +0200, Andi Shyti wrote:
> Hi Heikki,
> 
> On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> > The regmap is coming from the parent also in case of Xe
> > GPUs. Reusing the Wangxun quirk for that.
> 
> What I don't like, though, is that there is no mention of the
> change in the probe in the commit log.
> 
> Besides, are these changes related? Can we have them split in two
> different patches? I guess this would also make Andy's point
> about the extra churn.

I think you are right about splitting the patch. I'll do just that.
Thanks for the review Andi.

-- 
heikki