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 | 297 ++++++++++++++++++++++++++
drivers/gpu/drm/xe/xe_i2c.h | 58 +++++
drivers/gpu/drm/xe/xe_irq.c | 2 +
11 files changed, 387 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 30ed74ad29ab..257bbb69d6ca 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 cd17c1354ab3..71356ee31e9d 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_memirq.h"
#include "xe_mmio.h"
@@ -925,6 +926,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 6aca4b1a2824..dffff2d80f18 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..bfbfe1de7f77
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -0,0 +1,297 @@
+/* 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.start = i2c->adapter_irq;
+ res.name = "xe_i2c";
+ res.flags = IORESOURCE_IRQ;
+
+ 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) {
+ 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;
+ 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 xe_i2c_endpoint ep;
+ struct regmap *regmap;
+ struct xe_i2c *i2c;
+ int ret;
+
+ xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
+ if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
+ return 0;
+
+ i2c = devm_kzalloc(xe->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 = xe->drm.dev;
+ i2c->ep = ep;
+
+ regmap = devm_regmap_init(i2c->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(i2c->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.2On Thu, Jun 26, 2025 at 04:56:07PM +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).
...
> +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.start = i2c->adapter_irq;
> + res.name = "xe_i2c";
> + res.flags = IORESOURCE_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 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);
Wondering if you need to setup a custom lockdep class here.
> + return 0;
> +}
...
> +static void xe_i2c_remove_irq(struct xe_i2c *i2c)
> +{
> + if (i2c->irqdomain) {
if (!i2c->irqdomain)
return;
> + irq_dispose_mapping(i2c->adapter_irq);
> + irq_domain_remove(i2c->irqdomain);
> + }
> +}
...
> +static void xe_i2c_remove(void *data)
> +{
> + struct xe_i2c *i2c = data;
> + int i;
unsigned?
> + 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);
> +}
...
> +int xe_i2c_probe(struct xe_device *xe)
> +{
> + struct xe_i2c_endpoint ep;
> + struct regmap *regmap;
> + struct xe_i2c *i2c;
> + int ret;
> +
> + xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
> + if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
> + return 0;
> +
> + i2c = devm_kzalloc(xe->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 = xe->drm.dev;
> + i2c->ep = ep;
> + regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config);
Use of i2c->drm_dev makes harder to maintain and understand the code.
Managed resources should be carefully attached to the correct device,
otherwise it's inevitable object lifetime related issues.
With
struct device *dev = xe->drm.dev;
and using local dev, it becomes easier to get and avoid such subtle mistakes.
> + 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(i2c->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;
> +}
--
With Best Regards,
Andy Shevchenko
Thanks for the review Andy.
On Thu, Jun 26, 2025 at 05:21:02PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 26, 2025 at 04:56:07PM +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).
>
> ...
>
> > +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.start = i2c->adapter_irq;
> > + res.name = "xe_i2c";
> > + res.flags = IORESOURCE_IRQ;
>
>
> struct resource res;
>
> res = DEFINE_RES_IRQ_NAMED(i2c->adapter_irq, "xe_i2c");
I have to check what happened during the internal review round,
because I used that originally. But If there's no real reason not to
continue with it, then yes, I'll use it of course.
> > + 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 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);
>
> Wondering if you need to setup a custom lockdep class here.
>
> > + return 0;
> > +}
>
> ...
>
> > +static void xe_i2c_remove_irq(struct xe_i2c *i2c)
> > +{
> > + if (i2c->irqdomain) {
>
> if (!i2c->irqdomain)
> return;
>
> > + irq_dispose_mapping(i2c->adapter_irq);
> > + irq_domain_remove(i2c->irqdomain);
> > + }
> > +}
>
> ...
>
> > +static void xe_i2c_remove(void *data)
> > +{
> > + struct xe_i2c *i2c = data;
> > + int i;
>
> unsigned?
>
> > + 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);
> > +}
>
> ...
>
> > +int xe_i2c_probe(struct xe_device *xe)
> > +{
> > + struct xe_i2c_endpoint ep;
> > + struct regmap *regmap;
> > + struct xe_i2c *i2c;
> > + int ret;
> > +
> > + xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
> > + if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
> > + return 0;
> > +
> > + i2c = devm_kzalloc(xe->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 = xe->drm.dev;
> > + i2c->ep = ep;
>
> > + regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config);
>
> Use of i2c->drm_dev makes harder to maintain and understand the code.
> Managed resources should be carefully attached to the correct device,
> otherwise it's inevitable object lifetime related issues.
>
> With
>
> struct device *dev = xe->drm.dev;
>
> and using local dev, it becomes easier to get and avoid such subtle mistakes.
I have to disagree with you on this one. Local dev pointers create
problems because of the assumption that there is only a single device
in the function to deal with (especially if they are named "dev"),
which is almost never the case - this function is no exception.
But I'll add the local variable as you requested - I'll just name it
carefully.
This kinda related but off topic. IMO in cases like this the regmap
should be assigned to the child device that is being created instead
of the parent device. That is currently prevented by the current
regmap API - the device has to be fully registered before the regmap
can be assigned (and I'm not referring to the resource managed devm_*
API), but I'm not convinced that it has to be like that. The problem
is that the parent device may have multiple child devices that each
need a dedicated regmag. So just as a note to self: check if we can
improve the regmap API.
cheers,
--
heikki
On Fri, Jun 27, 2025 at 11:56:51AM +0300, Heikki Krogerus wrote: > On Thu, Jun 26, 2025 at 05:21:02PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 26, 2025 at 04:56:07PM +0300, Heikki Krogerus wrote: ... > > > + regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config); > > > > Use of i2c->drm_dev makes harder to maintain and understand the code. > > Managed resources should be carefully attached to the correct device, > > otherwise it's inevitable object lifetime related issues. > > > > With > > > > struct device *dev = xe->drm.dev; > > > > and using local dev, it becomes easier to get and avoid such subtle mistakes. > > I have to disagree with you on this one. Local dev pointers create > problems because of the assumption that there is only a single device > in the function to deal with (especially if they are named "dev"), > which is almost never the case - this function is no exception. Hmm... In my experience more than 70% of the drivers are okay with this approach as they do *not* use multiple device pointers. Usually, if we take IIO for the example, they have a physical device, which is depicted by the local variable named 'dev' and a Linux IIO device, which is container that has struct device inside, but it's not used explicitly, IIO APIs use the pointer to the container (and of course its name differs). That said, it seems we have different experience (you are most likely talking about USB cases, where that is indeed quite complicated already). > But I'll add the local variable as you requested - I'll just name it > carefully. Sure. ... > This kinda related but off topic. IMO in cases like this the regmap > should be assigned to the child device that is being created instead > of the parent device. That is currently prevented by the current > regmap API - the device has to be fully registered before the regmap > can be assigned (and I'm not referring to the resource managed devm_* > API), but I'm not convinced that it has to be like that. The problem > is that the parent device may have multiple child devices that each > need a dedicated regmag. So just as a note to self: check if we can > improve the regmap API. I'm not sure I follow. The (some of) MFD drivers, for instance, work with many children and one regmapi that is split over the devices. I don't see any issue with that. The problem is when one tries to mix regmap and non-regmap approaches in the same (big) driver. That's a road to mine field. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.