Re: [PATCH v1 1/1] drm/xe/sysctrl: Add system controller component for Xe3p dGPU platforms

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Dec 30, 2025 at 07:17:23AM -0800, Anoop, Vijay wrote:
>From: Anoop Vijay <anoop.c.vijay@intel.com>
>
>Add a new system controller (sysctrl) component for Intel Xe3p dGPU platforms.
>
>This component provides the foundational infrastructure for communication
>with the System Controller firmware using MKHI protocol over a mailbox interface.
>
>Key features introduced:
> - Detection and initialization of System Controller interface on Xe3p dGPU platforms
> - Mailbox communication with System Controller firmware
> - Fragmented message transfer for large command payloads
>
>This implementation establishes the base for future System Controller feature
>enablement and firmware command handling.
>
>Signed-off-by: Anoop Vijay <anoop.c.vijay@intel.com>

Commit message should follow '75 chars per line' format. In general, 
please look at the CI.Hooks and CI.checkpatch warnings/errors and fix 
those.

checkpatch can be run before posting:
https://docs.kernel.org/dev-tools/checkpatch.html

>---
> drivers/gpu/drm/xe/Makefile               |   2 +
> drivers/gpu/drm/xe/regs/xe_sysctrl_regs.h |  44 +++
> drivers/gpu/drm/xe/xe_device.c            |   5 +
> drivers/gpu/drm/xe/xe_device_types.h      |   6 +
> drivers/gpu/drm/xe/xe_pci.c               |   2 +
> drivers/gpu/drm/xe/xe_pci_types.h         |   1 +
> drivers/gpu/drm/xe/xe_sysctrl.c           |  64 ++++
> drivers/gpu/drm/xe/xe_sysctrl.h           |  13 +
> drivers/gpu/drm/xe/xe_sysctrl_mailbox.c   | 438 ++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_sysctrl_mailbox.h   |  61 +++
> drivers/gpu/drm/xe/xe_sysctrl_types.h     |  25 ++
> 11 files changed, 661 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/regs/xe_sysctrl_regs.h
> create mode 100644 drivers/gpu/drm/xe/xe_sysctrl.c
> create mode 100644 drivers/gpu/drm/xe/xe_sysctrl.h
> create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_types.h
>
>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>index 3315f93a35b2..cd03e4c53127 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -121,6 +121,8 @@ xe-y += xe_bb.o \
> 	xe_step.o \
> 	xe_survivability_mode.o \
> 	xe_sync.o \
>+	xe_sysctrl.o \
>+	xe_sysctrl_mailbox.o \
> 	xe_tile.o \
> 	xe_tile_sysfs.o \
> 	xe_tlb_inval.o \
>diff --git a/drivers/gpu/drm/xe/regs/xe_sysctrl_regs.h b/drivers/gpu/drm/xe/regs/xe_sysctrl_regs.h
>new file mode 100644
>index 000000000000..b31b6e24c18e
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/regs/xe_sysctrl_regs.h
>@@ -0,0 +1,44 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#ifndef _XE_SYSCTRL_REGS_H_
>+#define _XE_SYSCTRL_REGS_H_
>+
>+#include "xe_regs.h"
>+
>+#define SYSCTRL_BASE_OFFSET		0xDB000
>+#define SYSCTRL_BASE			(SOC_BASE + SYSCTRL_BASE_OFFSET)
>+#define SYSCTRL_MAILBOX_INDEX		0x03
>+#define SC_BAR_LENGTH			0x1000
>+
>+#define SC_MB_CTRL			XE_REG(SYSCTRL_BASE + 0x10)
>+#define   SC_MB_CTRL_RUN_BUSY		REG_BIT(31)
>+#define   SC_MB_CTRL_IRQ		REG_BIT(30)
>+#define   SC_MB_CTRL_RUN_BUSY_OUT	REG_BIT(29)
>+#define   SC_MB_CTRL_PARAM3_MASK	REG_GENMASK(28, 24)
>+#define   SC_MB_CTRL_PARAM2_MASK	REG_GENMASK(23, 16)
>+#define   SC_MB_CTRL_PARAM1_MASK	REG_GENMASK(15, 8)
>+#define   SC_MB_CTRL_COMMAND_MASK	REG_GENMASK(7, 0)
>+
>+#define SC_MB_DATA0			XE_REG(SYSCTRL_BASE + 0x14)
>+#define SC_MB_DATA1			XE_REG(SYSCTRL_BASE + 0x18)
>+#define SC_MB_DATA2			XE_REG(SYSCTRL_BASE + 0x1C)
>+#define SC_MB_DATA3			XE_REG(SYSCTRL_BASE + 0x20)
>+
>+#define MKHI_FRAME_PHASE		REG_BIT(24)
>+#define MKHI_FRAME_CURRENT_MASK		REG_GENMASK(21, 16)
>+#define MKHI_FRAME_TOTAL_MASK		REG_GENMASK(13, 8)
>+#define MKHI_FRAME_COMMAND_MASK		REG_GENMASK(7, 0)
>+
>+#define SC_MB_FRAME_SIZE		16
>+#define SC_MB_MAX_FRAMES		64
>+#define SC_MB_MAX_MESSAGE_SIZE		(SC_MB_FRAME_SIZE * SC_MB_MAX_FRAMES)
>+#define SC_MKHI_COMMAND			5
>+
>+#define SC_MB_DEFAULT_TIMEOUT_MS	500
>+#define SC_MB_RETRY_TIMEOUT_MS		20
>+#define SC_MB_POLL_INTERVAL_US		100
>+
>+#endif /* _XE_SYSCTRL_REGS_H_ */
>diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>index e101d290b2a6..805d48dd954d 100644
>--- a/drivers/gpu/drm/xe/xe_device.c
>+++ b/drivers/gpu/drm/xe/xe_device.c
>@@ -66,6 +66,7 @@
> #include "xe_survivability_mode.h"
> #include "xe_sriov.h"
> #include "xe_svm.h"
>+#include "xe_sysctrl.h"
> #include "xe_tile.h"
> #include "xe_ttm_stolen_mgr.h"
> #include "xe_ttm_sys_mgr.h"
>@@ -1032,6 +1033,10 @@ int xe_device_probe(struct xe_device *xe)
> 	if (err)
> 		goto err_unregister_display;
>
>+	err = xe_sysctrl_init(xe);
>+	if (err)
>+		goto err_unregister_display;
>+
> 	err = xe_device_sysfs_init(xe);
> 	if (err)
> 		goto err_unregister_display;
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index a85be9ba175e..6295b2c35d4a 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -29,6 +29,7 @@
> #include "xe_sriov_vf_ccs_types.h"
> #include "xe_step_types.h"
> #include "xe_survivability_mode_types.h"
>+#include "xe_sysctrl_types.h"
> #include "xe_tile_sriov_vf_types.h"
> #include "xe_validation.h"
>
>@@ -340,6 +341,8 @@ struct xe_device {
> 		u8 has_soc_remapper_telem:1;
> 		/** @info.has_sriov: Supports SR-IOV */
> 		u8 has_sriov:1;
>+		/** @info.has_sysctrl: Supports System Controller */
>+		u8 has_sysctrl:1;
> 		/** @info.has_usm: Device has unified shared memory support */
> 		u8 has_usm:1;
> 		/** @info.has_64bit_timestamp: Device supports 64-bit timestamps */
>@@ -606,6 +609,9 @@ struct xe_device {
> 	/** @heci_gsc: graphics security controller */
> 	struct xe_heci_gsc heci_gsc;
>
>+	/** @sc: System Controller */
>+	struct xe_sysctrl sc;
>+
> 	/** @nvm: discrete graphics non-volatile memory */
> 	struct intel_dg_nvm_dev *nvm;
>
>diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>index 91e0553a8163..b6dc3030b673 100644
>--- a/drivers/gpu/drm/xe/xe_pci.c
>+++ b/drivers/gpu/drm/xe/xe_pci.c
>@@ -426,6 +426,7 @@ static const struct xe_device_desc cri_desc = {
> 	.has_soc_remapper_sysctrl = true,
> 	.has_soc_remapper_telem = true,
> 	.has_sriov = true,
>+	.has_sysctrl = true,
> 	.max_gt_per_tile = 2,
> 	.require_force_probe = true,
> 	.va_bits = 57,
>@@ -701,6 +702,7 @@ static int xe_info_init_early(struct xe_device *xe,
> 	xe->info.has_soc_remapper_telem = desc->has_soc_remapper_telem;
> 	xe->info.has_sriov = xe_configfs_primary_gt_allowed(to_pci_dev(xe->drm.dev)) &&
> 		desc->has_sriov;
>+	xe->info.has_sysctrl = desc->has_sysctrl;
> 	xe->info.has_mem_copy_instr = desc->has_mem_copy_instr;
> 	xe->info.skip_guc_pc = desc->skip_guc_pc;
> 	xe->info.skip_mtcfg = desc->skip_mtcfg;
>diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
>index 5f20f56571d1..53e44a32883d 100644
>--- a/drivers/gpu/drm/xe/xe_pci_types.h
>+++ b/drivers/gpu/drm/xe/xe_pci_types.h
>@@ -56,6 +56,7 @@ struct xe_device_desc {
> 	u8 has_soc_remapper_sysctrl:1;
> 	u8 has_soc_remapper_telem:1;
> 	u8 has_sriov:1;
>+	u8 has_sysctrl:1;
> 	u8 needs_scratch:1;
> 	u8 skip_guc_pc:1;
> 	u8 skip_mtcfg:1;
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl.c b/drivers/gpu/drm/xe/xe_sysctrl.c
>new file mode 100644
>index 000000000000..9f3e0b1db380
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_sysctrl.c
>@@ -0,0 +1,64 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#include "xe_sysctrl.h"

The above header should just be placed in alphabetical order below.

>+
>+#include <linux/device.h>
>+#include <linux/mutex.h>
>+
>+#include <drm/drm_managed.h>
>+
>+#include "regs/xe_sysctrl_regs.h"
>+#include "xe_device.h"
>+#include "xe_printk.h"
>+#include "xe_soc_remapper.h"
>+#include "xe_sysctrl_mailbox.h"
>+#include "xe_sysctrl_types.h"
>+
>+static void xe_sysctrl_fini(void *arg)
>+{
>+	struct xe_sysctrl *sc = arg;
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);

You could use a helper and replace it everywhere with sc_to_xe(sc):

static struct xe_device *sc_to_xe(struct xe_sysctrl *sc)
{
	return container_of(sc, struct xe_device, sc);
}

>+
>+	if (!xe->soc_remapper.set_sysctrl_region)
>+		return;
>+
>+	xe->soc_remapper.set_sysctrl_region(xe, 0);
>+}
>+
>+/**
>+ * xe_sysctrl_init - Initialize SC subsystem
>+ * @xe: xe device instance
>+ *
>+ * Entry point for SC initialization, called from xe_device_probe().
>+ * This function checks platform support and initializes the system controller.
>+ *
>+ * Return: 0 on success, error code on failure
>+ */
>+int xe_sysctrl_init(struct xe_device *xe)
>+{
>+	struct xe_sysctrl *sc = &xe->sc;
>+	int ret;
>+
>+	if (!xe->info.has_sysctrl)
>+		return 0;
>+
>+	ret = devm_add_action_or_reset(xe->drm.dev, xe_sysctrl_fini, sc);
>+	if (ret)
>+		return ret;
>+
>+	if (!xe->soc_remapper.set_sysctrl_region)
>+		return -ENODEV;
>+
>+	xe->soc_remapper.set_sysctrl_region(xe, SYSCTRL_MAILBOX_INDEX);
>+
>+	ret = drmm_mutex_init(&xe->drm, &sc->cmd_lock);
>+	if (ret)
>+		return ret;
>+
>+	xe_sysctrl_mailbox_init(sc);
>+
>+	return 0;
>+}
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl.h b/drivers/gpu/drm/xe/xe_sysctrl.h
>new file mode 100644
>index 000000000000..f1ad12656e48
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_sysctrl.h
>@@ -0,0 +1,13 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#ifndef _XE_SYSCTRL_H_
>+#define _XE_SYSCTRL_H_
>+
>+struct xe_device;
>+
>+int xe_sysctrl_init(struct xe_device *xe);
>+
>+#endif /* _XE_SYSCTRL_H_ */
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>new file mode 100644
>index 000000000000..e9f4d4b13bb1
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>@@ -0,0 +1,438 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#include <linux/bitfield.h>
>+#include <linux/errno.h>
>+#include <linux/minmax.h>
>+#include <linux/mutex.h>
>+#include <linux/slab.h>
>+#include <linux/string.h>
>+#include <linux/types.h>
>+
>+#include <drm/drm_print.h>

Do you still need this one? Can you also double check if all headers 
here are needed.

>+
>+#include "xe_device.h"
>+#include "xe_mmio.h"
>+#include "xe_pm.h"
>+#include "xe_printk.h"
>+#include "xe_sysctrl.h"
>+#include "xe_sysctrl_mailbox.h"
>+#include "xe_sysctrl_types.h"
>+#include "regs/xe_sysctrl_regs.h"
>+
>+static bool xe_sysctrl_mailbox_wait_bit_clear(struct xe_sysctrl *sc, u32 bit_mask,
>+					      unsigned int timeout_ms)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	int ret;
>+
>+	ret = xe_mmio_wait32_not(xe_root_tile_mmio(xe), SC_MB_CTRL, bit_mask, bit_mask,
>+				 timeout_ms * 1000, NULL, false);
>+
>+	return ret == 0;
>+}
>+
>+static bool xe_sysctrl_mailbox_wait_bit_set(struct xe_sysctrl *sc, u32 bit_mask,
>+					    unsigned int timeout_ms)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	int ret;
>+
>+	ret = xe_mmio_wait32(xe_root_tile_mmio(xe), SC_MB_CTRL, bit_mask, bit_mask,
>+			     timeout_ms * 1000, NULL, false);
>+
>+	return ret == 0;
>+}
>+
>+static int xe_sysctrl_mailbox_write_frame(struct xe_sysctrl *sc, const void *frame,
>+					  size_t len)
>+{
>+	static const struct xe_reg data_regs[] = {
>+		SC_MB_DATA0, SC_MB_DATA1, SC_MB_DATA2, SC_MB_DATA3
>+	};
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	const u8 *bytes = frame;
>+	u32 reg_data;
>+	size_t i;
>+
>+	if (len == 0 || len > SC_MB_FRAME_SIZE) {
>+		xe_err(xe, "sysctrl: Invalid frame len: %zu\n", len);
>+		return -EINVAL;
>+	}

This check seems redundant. You are already rounding up cmd_size to 
frames and then the frame size passed to this function can only be up to 
SC_MB_FRAME_SIZE, but not zero. If this function were called from 
outside this file, then additional checks makes sense, but it's static, 
so this check can be dropped.

>+
>+	for (i = 0; i + sizeof(u32) <= len; i += sizeof(u32)) {
>+		reg_data = *(const u32 *)(bytes + i);
>+		xe_mmio_write32(xe_root_tile_mmio(xe), data_regs[i / sizeof(u32)], reg_data);
>+	}
>+
>+	if (i < len) {
>+		size_t remaining = len - i;
>+		size_t j;
>+
>+		reg_data = 0;
>+		for (j = 0; j < remaining; j++)
>+			reg_data |= (u32)bytes[i + j] << (j * 8);
>+
>+		xe_mmio_write32(xe_root_tile_mmio(xe), data_regs[i / sizeof(u32)], reg_data);
>+	}

I would suggest to simplify the logic here to make it more readable, something
like:

static const struct xe_reg regs[] = {
	SC_MB_DATA0, SC_MB_DATA1, SC_MB_DATA2, SC_MB_DATA3
};
struct xe_device *xe = container_of(sc, typeof(*xe), sc);
struct xe_mmio *mmio = xe_root_tile_mmio(xe);
u32 dw = DIV_ROUND_UP(len, sizeof(u32));
u32 val[SC_MB_FRAME_SIZE/4] = {0}, i;

memcpy(val, frame, len);
for (i = 0; i < dw; i++)
	xe_mmio_write32(mmio, regs[i], val[i]);

return 0;

>+
>+	return 0;
>+}
>+
>+static int xe_sysctrl_mailbox_read_frame(struct xe_sysctrl *sc, void *frame,
>+					 size_t len)
>+{
>+	static const struct xe_reg data_regs[] = {
>+		SC_MB_DATA0, SC_MB_DATA1, SC_MB_DATA2, SC_MB_DATA3
>+	};
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	u8 *bytes = frame;
>+	u32 reg_data;
>+	size_t i;
>+
>+	if (len == 0 || len > SC_MB_FRAME_SIZE) {
>+		xe_err(xe, "sysctrl: Invalid frame len: %zu\n", len);
>+		return -EINVAL;
>+	}
>+
>+	for (i = 0; i + sizeof(u32) <= len; i += sizeof(u32)) {
>+		reg_data = xe_mmio_read32(xe_root_tile_mmio(xe), data_regs[i / sizeof(u32)]);
>+		*(u32 *)(bytes + i) = reg_data;
>+	}
>+
>+	if (i < len) {
>+		size_t remaining = len - i;
>+		size_t j;
>+
>+		reg_data = xe_mmio_read32(xe_root_tile_mmio(xe), data_regs[i / sizeof(u32)]);
>+
>+		for (j = 0; j < remaining; j++)
>+			bytes[i + j] = (reg_data >> (j * 8)) & 0xFF;
>+	}

A logic similar to the write frame can likely be used to simplify this function.

static const struct xe_reg regs[] = {
	SC_MB_DATA0, SC_MB_DATA1, SC_MB_DATA2, SC_MB_DATA3
};
struct xe_device *xe = container_of(sc, typeof(*xe), sc);
struct xe_mmio *mmio = xe_root_tile_mmio(xe);
u32 dw = DIV_ROUND_UP(len, sizeof(u32));
u32 val[SC_MB_FRAME_SIZE/4] = {0}, i;

for (i = 0; i < dw; i++)
	val[i] = xe_mmio_read32(mmio, regs[i]);

memcpy(frame, val, len);

return 0;
>+
>+	return 0;
>+}
>+
>+static void xe_sysctrl_mailbox_clear_response(struct xe_sysctrl *sc)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	u32 ctrl_reg;
>+
>+	ctrl_reg = xe_mmio_read32(xe_root_tile_mmio(xe), SC_MB_CTRL);
>+	ctrl_reg &= ~SC_MB_CTRL_RUN_BUSY_OUT;
>+	xe_mmio_write32(xe_root_tile_mmio(xe), SC_MB_CTRL, ctrl_reg);

You could use xe_mmio_rmw32() instead.

>+}
>+
>+static int xe_sysctrl_mailbox_prepare_command(struct xe_sysctrl *sc,
>+					      u8 group_id, u8 command,
>+					       const void *data_in, size_t data_in_len,
>+					       u8 **cmd_buffer, size_t *cmd_size)
>+{

In the caller cmd_buffer has a different type and here it is u8**. For 
readability, I would recommend using a different name here, maybe 
mkhi_msg.

>+	struct xe_sysctrl_mailbox_mkhi_msg_hdr *mkhi_hdr;
>+	size_t size;
>+	u8 *buffer;
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);

I would move the xe initialization to the top.

>+
>+	size = sizeof(*mkhi_hdr) + data_in_len;
>+	if (size > SC_MB_MAX_MESSAGE_SIZE) {
>+		xe_err(xe, "sysctrl: Message too large: %zu bytes\n", size);
>+		return -EINVAL;
>+	}
>+
>+	buffer = kmalloc(size, GFP_KERNEL);
>+	if (!buffer)
>+		return -ENOMEM;
>+
>+	mkhi_hdr = (struct xe_sysctrl_mailbox_mkhi_msg_hdr *)buffer;
>+	mkhi_hdr->data = cpu_to_le32(FIELD_PREP(MKHI_HDR_GROUP_ID_MASK, group_id) |
>+				      FIELD_PREP(MKHI_HDR_COMMAND_MASK, command & 0x7F) |
>+				      FIELD_PREP(MKHI_HDR_IS_RESPONSE, 0) |
>+				      FIELD_PREP(MKHI_HDR_RESERVED_MASK, 0) |
>+				      FIELD_PREP(MKHI_HDR_RESULT_MASK, 0));
>+

FIELD_PREPs un-aligned by one space.

>+	if (data_in && data_in_len)
>+		memcpy(buffer + sizeof(*mkhi_hdr), data_in, data_in_len);
>+
>+	*cmd_buffer = buffer;
>+	*cmd_size = size;
>+
>+	return 0;
>+}
>+
>+static int xe_sysctrl_mailbox_send_frames(struct xe_sysctrl *sc, const u8 *cmd_buffer,
>+					  size_t cmd_size, unsigned int timeout_ms)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	u32 ctrl_reg, total_frames, frame;
>+	size_t bytes_sent, frame_size;
>+
>+	total_frames = DIV_ROUND_UP(cmd_size, SC_MB_FRAME_SIZE);
>+
>+	if (!xe_sysctrl_mailbox_wait_bit_clear(sc, SC_MB_CTRL_RUN_BUSY, timeout_ms)) {
>+		xe_err(xe, "sysctrl: Mailbox busy\n");
>+		return -EBUSY;
>+	}
>+
>+	sc->phase_bit ^= 1;
>+
Empty line not needed here.
>+	bytes_sent = 0;
>+
>+	for (frame = 0; frame < total_frames; frame++) {
>+		frame_size = min(cmd_size - bytes_sent, (size_t)SC_MB_FRAME_SIZE);
>+
>+		if (xe_sysctrl_mailbox_write_frame(sc, cmd_buffer + bytes_sent, frame_size)) {
>+			xe_err(xe, "sysctrl: Failed to write frame %u\n", frame);
>+			return -EIO;

If you failed to write the frame here, what happens to phase bit. Should 
you toggle it back?

>+		}
>+
>+		ctrl_reg = SC_MB_CTRL_RUN_BUSY |
>+			   FIELD_PREP(MKHI_FRAME_CURRENT_MASK, frame) |
>+			   FIELD_PREP(MKHI_FRAME_TOTAL_MASK, total_frames - 1) |
>+			   FIELD_PREP(MKHI_FRAME_COMMAND_MASK, SC_MKHI_COMMAND) |
>+			   (sc->phase_bit ? MKHI_FRAME_PHASE : 0);
>+
>+		xe_mmio_write32(xe_root_tile_mmio(xe), SC_MB_CTRL, ctrl_reg);
>+
>+		if (!xe_sysctrl_mailbox_wait_bit_clear(sc, SC_MB_CTRL_RUN_BUSY, timeout_ms)) {
>+			xe_err(xe, "sysctrl: Frame %u acknowledgment timeout\n", frame);
>+			return -ETIMEDOUT;
>+		}
>+
>+		bytes_sent += frame_size;
>+	}
>+
>+	return 0;
>+}
>+
>+static int xe_sysctrl_mailbox_process_first_frame(struct xe_sysctrl *sc,
>+						  const struct xe_sysctrl_mailbox_mkhi_msg_hdr *req_hdr,
>+						  void *out,
>+						  size_t frame_size,
>+						  size_t *payload_bytes)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	u32 frame_data[4];
>+	struct xe_sysctrl_mailbox_mkhi_msg_hdr *resp_hdr;
>+	size_t hdr_size = sizeof(*resp_hdr);
>+	size_t payload_size;
>+	int ret;
>+
>+	ret = xe_sysctrl_mailbox_read_frame(sc, frame_data, frame_size);
>+	if (ret)
>+		return ret;
>+
>+	resp_hdr = (struct xe_sysctrl_mailbox_mkhi_msg_hdr *)frame_data;
>+
>+	if (!XE_SYSCTRL_MKHI_HDR_IS_RESPONSE(resp_hdr) ||
>+	    XE_SYSCTRL_MKHI_HDR_GROUP_ID(resp_hdr) != XE_SYSCTRL_MKHI_HDR_GROUP_ID(req_hdr) ||
>+	    XE_SYSCTRL_MKHI_HDR_COMMAND(resp_hdr) != XE_SYSCTRL_MKHI_HDR_COMMAND(req_hdr)) {
>+		xe_err(xe, "SC: Response header mismatch\n");
>+		return -EPROTO;
>+	}
>+
>+	if (XE_SYSCTRL_MKHI_HDR_RESULT(resp_hdr) != 0) {
>+		xe_err(xe, "SC: Firmware error: 0x%02lx\n",
>+		       XE_SYSCTRL_MKHI_HDR_RESULT(resp_hdr));
>+		return -EIO;
>+	}
>+
>+	payload_size = frame_size - hdr_size;
>+	if (payload_size > 0)
>+		memcpy(out, (u8 *)frame_data + hdr_size, payload_size);
>+
>+	*payload_bytes = payload_size;
>+
>+	xe_sysctrl_mailbox_clear_response(sc);
>+
>+	return 0;
>+}
>+
>+static int xe_sysctrl_mailbox_process_frame(struct xe_sysctrl *sc,
>+					    void *out, size_t frame_size,
>+					     unsigned int timeout_ms)
>+{
>+	int ret;
>+
>+	if (!xe_sysctrl_mailbox_wait_bit_set(sc, SC_MB_CTRL_RUN_BUSY_OUT, timeout_ms))

response frame timedout message must be here.

>+		return -ETIMEDOUT;
>+
>+	ret = xe_sysctrl_mailbox_read_frame(sc, out, frame_size);
>+	if (ret)
>+		return ret;
>+
>+	xe_sysctrl_mailbox_clear_response(sc);
>+
>+	return 0;
>+}
>+
>+static int xe_sysctrl_mailbox_receive_frames(struct xe_sysctrl *sc,
>+					     const struct xe_sysctrl_mailbox_mkhi_msg_hdr *req_hdr,
>+					      void *data_out, size_t data_out_len,
>+					      size_t *rdata_len, unsigned int timeout_ms)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	struct xe_sysctrl_mailbox_mkhi_msg_hdr *mkhi_hdr;
>+	u32 ctrl_reg, total_frames, frame;
>+	size_t hdr_size = sizeof(*mkhi_hdr);
>+	u8 *out = data_out;
>+	size_t received = 0;
>+	size_t frame_size;
>+	int ret;

int ret = 0;

>+
>+	if (!xe_sysctrl_mailbox_wait_bit_set(sc, SC_MB_CTRL_RUN_BUSY_OUT, timeout_ms)) {
>+		xe_err(xe, "sysctrl: Response frame 0 timeout\n");
>+		return -ETIMEDOUT;
>+	}
>+
>+	ctrl_reg = xe_mmio_read32(xe_root_tile_mmio(xe), SC_MB_CTRL);
>+	total_frames = FIELD_GET(MKHI_FRAME_TOTAL_MASK, ctrl_reg) + 1;
>+
>+	if (total_frames == 1)
>+		frame_size = min(hdr_size + data_out_len, (size_t)SC_MB_FRAME_SIZE);
>+	else
>+		frame_size = SC_MB_FRAME_SIZE;
>+
>+	ret = xe_sysctrl_mailbox_process_first_frame(sc, req_hdr, out, frame_size, &received);
>+	if (ret)
>+		return ret;
>+
>+	out += received;
>+
>+	for (frame = 1; frame < total_frames; frame++) {
>+		size_t remaining = data_out_len - received;
>+
>+		frame_size = min_t(size_t, remaining, SC_MB_FRAME_SIZE);
>+
>+		ret = xe_sysctrl_mailbox_process_frame(sc, out, frame_size, timeout_ms);
>+		if (ret) {
>+			xe_err(xe, "sysctrl: Response frame %u timeout\n", frame);

this message should be in xe_sysctrl_mailbox_process_frame.

At this point you have already copied some data into the out pointer, so 
you might as well update *rdata_len with the partial copied data. So you 
can break here and return ret at the end of the function.

>+			return ret;
>+		}
>+
>+		received += frame_size;
>+		out += frame_size;
>+	}
>+
>+	*rdata_len = received;
>+
>+	return 0;

return ret;

>+}
>+
>+static int xe_sysctrl_mailbox_send_command(struct xe_sysctrl *sc,
>+					   const u8 *cmd_buffer, size_t cmd_size,
>+					    void *data_out, size_t data_out_len,
>+					    size_t *rdata_len, unsigned int timeout_ms)
>+{
>+	const struct xe_sysctrl_mailbox_mkhi_msg_hdr *mkhi_hdr;
>+	size_t received;
>+	int ret;
>+
>+	ret = xe_sysctrl_mailbox_send_frames(sc, cmd_buffer, cmd_size, timeout_ms);
>+	if (ret)
>+		return ret;
>+
>+	if (!data_out) {
>+		if (rdata_len)
>+			*rdata_len = 0;
>+		return 0;
>+	}

The above check should just be

	if (!data_out || !rdata_len)
		return 0;

I would recommend writing to *rdata_len only on successful receive.

>+
>+	mkhi_hdr = (const struct xe_sysctrl_mailbox_mkhi_msg_hdr *)cmd_buffer;
>+
>+	ret = xe_sysctrl_mailbox_receive_frames(sc, mkhi_hdr, data_out, data_out_len,
>+						&received, timeout_ms);
>+	if (ret)
>+		return ret;
>+
>+	if (rdata_len)
>+		*rdata_len = received;

If you added the above comment, then at this point you already know rdata_len
is valid, so the check can be dropped.

>+
>+	return 0;
>+}
>+
>+/**
>+ * xe_sysctrl_send_command - Send command to System Controller via mailbox
>+ * @handle: XE device handle
>+ * @cmd_buffer: Pointer to xe_sysctrl_mailbox_command structure
>+ * @rdata_len: Pointer to store actual response data size (can be NULL)
>+ *
>+ * Send a command to the System Controller using MKHI protocol. Handles
>+ * command preparation, fragmentation, transmission, and response reception.
>+ *
>+ * Return: 0 on success, negative error code on failure
>+ */
>+int xe_sysctrl_send_command(void *handle, void *cmd_buffer, size_t *rdata_len)
>+{

Why void *cmd_buffer? Do you expect anything other than 
xe_sysctrl_mailbox_command? If you do, your function signature will 
change anyways, so why not just define it as struct 
xe_sysctrl_mailbox_command *cmd?

>+	struct xe_device *xe = handle;
>+	struct xe_sysctrl *sc = &xe->sc;
>+	struct xe_sysctrl_mailbox_command *cmd = cmd_buffer;
>+	u8 *buffer = NULL;
>+	size_t command_size = 0;
>+	u8 group_id, command_code;
>+	int ret = 0;
>+
>+	if (!xe || !cmd) {

I suggest splitting the 2 checks here into separate if blocks and use 
pr_err or pr_debug if xe is NULL.

>+		if (xe)
>+			xe_err(xe, "sysctrl: Invalid parameters\n");
>+		return -EINVAL;
>+	}
>+
>+	if (!xe->info.has_sysctrl)
>+		return -ENODEV;
>+
>+	group_id = XE_SYSCTRL_APP_HDR_GROUP_ID(&cmd->header);
>+	command_code = XE_SYSCTRL_APP_HDR_COMMAND(&cmd->header);

Would be good to check if the group_id and command_id are valid here.
>+
>+	if (!cmd->data_in && cmd->data_in_len) {
>+		xe_err(xe, "sysctrl: Invalid input parameters\n");
>+		return -EINVAL;
>+	}
>+
>+	if (!cmd->data_out && cmd->data_out_len) {
>+		xe_err(xe, "sysctrl: Invalid output parameters\n");
>+		return -EINVAL;
>+	}
>+
>+	might_sleep();
>+
>+	ret = xe_sysctrl_mailbox_prepare_command(sc, group_id, command_code,
>+						 cmd->data_in, cmd->data_in_len,
>+						 &buffer, &command_size);
>+	if (ret) {
>+		xe_err(xe, "sysctrl: Failed to prepare command: %d\n", ret);
>+		return ret;
>+	}
>+
>+	xe_pm_runtime_get(xe);
>+
>+	guard(mutex)(&sc->cmd_lock);
>+
>+	ret = xe_sysctrl_mailbox_send_command(sc, buffer, command_size,
>+					      cmd->data_out, cmd->data_out_len, rdata_len,
>+					      SC_MB_DEFAULT_TIMEOUT_MS);
>+	if (ret)
>+		xe_err(xe, "sysctrl: Mailbox command failed: %d\n", ret);
>+
>+	xe_pm_runtime_put(xe);

hmm, not sure this ordering is correct. Ideally you want mutex_unlock 
before runtime pm put, but the guard will only make sure unlock happens 
at function return. If using guard, you may want to define a similar 
guard for runtime pm. Maybe that's what Michal was suggesting earlier.

>+
>+	kfree(buffer);
>+
>+	return ret;
>+}
>+
>+/**
>+ * xe_sysctrl_mailbox_init - Initialize the system controller mailbox state
>+ * @sysctrl: System controller structure
>+ */
>+void xe_sysctrl_mailbox_init(struct xe_sysctrl *sc)
>+{
>+	struct xe_device *xe = container_of(sc, typeof(*xe), sc);
>+	u32 ctrl_reg;
>+
>+	ctrl_reg = xe_mmio_read32(xe_root_tile_mmio(xe), SC_MB_CTRL);
>+	sc->phase_bit = (ctrl_reg & MKHI_FRAME_PHASE) ? 1 : 0;
>+
>+	xe_mmio_write32(xe_root_tile_mmio(xe), SC_MB_CTRL, 0);

You could use xe_mmio_rmw32() instead.

>+}
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>new file mode 100644
>index 000000000000..6380d1d68f8f
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>@@ -0,0 +1,61 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#ifndef __XE_SYSCTRL_MAILBOX_H__
>+#define __XE_SYSCTRL_MAILBOX_H__
>+
>+#include <linux/bitfield.h>
>+#include <linux/types.h>
>+
>+struct xe_sysctrl;
>+
>+#define MKHI_HDR_GROUP_ID_MASK		GENMASK(7, 0)
>+#define MKHI_HDR_COMMAND_MASK		GENMASK(14, 8)
>+#define MKHI_HDR_IS_RESPONSE		BIT(15)
>+#define MKHI_HDR_RESERVED_MASK		GENMASK(23, 16)
>+#define MKHI_HDR_RESULT_MASK		GENMASK(31, 24)

My interpretation was different from reading the spec. More like 
group_id is at bit[31:24] and result at bit[7:0]. Have you confirmed the 
above bitmask?

>+
>+struct xe_sysctrl_mailbox_mkhi_msg_hdr {
>+	__le32 data;
>+} __packed;
>+
>+#define APP_HDR_GROUP_ID_MASK		GENMASK(7, 0)
>+#define APP_HDR_COMMAND_MASK		GENMASK(15, 8)
>+#define APP_HDR_VERSION_MASK		GENMASK(23, 16)
>+#define APP_HDR_RESERVED_MASK		GENMASK(31, 24)
>+
>+struct xe_sysctrl_mailbox_app_msg_hdr {
>+	__le32 data;
>+} __packed;
>+
>+#define XE_SYSCTRL_APP_HDR_GROUP_ID(hdr)	FIELD_GET(APP_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>+#define XE_SYSCTRL_APP_HDR_COMMAND(hdr)		FIELD_GET(APP_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>+#define XE_SYSCTRL_APP_HDR_VERSION(hdr)		FIELD_GET(APP_HDR_VERSION_MASK, le32_to_cpu((hdr)->data))
>+
>+#define XE_SYSCTRL_MKHI_HDR_GROUP_ID(hdr)	FIELD_GET(MKHI_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>+#define XE_SYSCTRL_MKHI_HDR_COMMAND(hdr)	FIELD_GET(MKHI_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>+#define XE_SYSCTRL_MKHI_HDR_IS_RESPONSE(hdr)	FIELD_GET(MKHI_HDR_IS_RESPONSE, le32_to_cpu((hdr)->data))
>+#define XE_SYSCTRL_MKHI_HDR_RESULT(hdr)		FIELD_GET(MKHI_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>+
>+/**
>+ * struct xe_sysctrl_mailbox_command - System Controller mailbox command structure
>+ */
>+struct xe_sysctrl_mailbox_command {
>+	/** @header: Application message header containing command information */
>+	struct xe_sysctrl_mailbox_app_msg_hdr header;

>+	/** @data_in: Pointer to input payload data (can be NULL if no input data) */
>+	void *data_in;

>+	/** @data_in_len: Size of input payload in bytes (0 if no input data) */
>+	size_t data_in_len;

>+	/** @data_out: Pointer to output buffer for response data (can be NULL if no response) */
>+	void *data_out;

>+	/** @data_out_len: Size of output buffer in bytes (0 if no response expected) */
>+	size_t data_out_len;

Empty lines between members above will help readability.

>+};
>+
>+void xe_sysctrl_mailbox_init(struct xe_sysctrl *sc);
>+int xe_sysctrl_send_command(void *handle, void *cmd_buffer, size_t *rdata_len);
>+
>+#endif /* __XE_SYSCTRL_MAILBOX_H__ */
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl_types.h b/drivers/gpu/drm/xe/xe_sysctrl_types.h
>new file mode 100644
>index 000000000000..80e674cfa385
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_sysctrl_types.h
>@@ -0,0 +1,25 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+#ifndef _XE_SYSCTRL_TYPES_H_
>+#define _XE_SYSCTRL_TYPES_H_
>+
>+#include <linux/completion.h>
>+#include <linux/mutex.h>
>+#include <linux/types.h>
>+#include <linux/workqueue.h>

Use only what's needed in this header. If you C file requires some of 
these headers, you want to move those to the C file.

Regards,
Umesh

>+
>+/**
>+ * struct xe_sysctrl - System Controller driver context
>+ */
>+struct xe_sysctrl {
>+	/** @cmd_lock: Mutex protecting mailbox command operations */
>+	struct mutex cmd_lock;
>+
>+	/** @phase_bit: MKHI message boundary phase toggle bit */
>+	u32 phase_bit;
>+};
>+
>+#endif /* _XE_SYSCTRL_TYPES_H_ */
>-- 
>2.43.0
>