Re: [PATCH v7 2/6] drm/xe/sysctrl: Add System Controller types and structures

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 1/29/2026 1:10 PM, Anoop, Vijay wrote:
> From: Anoop Vijay <anoop.c.vijay@intel.com>
> 
> Add type definitions and header declarations for System
> Controller mailbox communication protocol.
> 
> Signed-off-by: Anoop Vijay <anoop.c.vijay@intel.com>
> ---
> v4: (Matt, Mike)
> - Add domain-specific MMIO accessor
> - Change phase_bit type from u32 to bool
> 
> v6: (Matt)
> - Add mailbox protocol constants
> ---
>  drivers/gpu/drm/xe/xe_sysctrl_mailbox.h       | 35 ++++++++++++++++
>  drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h | 42 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_sysctrl_types.h         | 33 +++++++++++++++
>  3 files changed, 110 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>  create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>  create mode 100644 drivers/gpu/drm/xe/xe_sysctrl_types.h
> 
> 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..2b64165c8e76
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef __XE_SYSCTRL_MAILBOX_H__
> +#define __XE_SYSCTRL_MAILBOX_H__

nit: in Xe we mostly use single underscore in guard names

> +
> +#include <linux/bitfield.h>
> +#include <linux/types.h>
> +
> +struct xe_sysctrl;
> +struct xe_device;
> +struct xe_sysctrl_mailbox_command;

please keep forward decls in order

> +
> +#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)

those definitions belong to ABI, no? see below

> +
> +#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))
> +
> +void xe_sysctrl_mailbox_init(struct xe_sysctrl *sc);
> +int xe_sysctrl_send_command(struct xe_device *xe,

why xe? this function should take sc

> +			    struct xe_sysctrl_mailbox_command *cmd,
> +			    size_t *rdata_len);

what's the point in adding function declarations if there no functions in this patch?

> +
> +#endif /* __XE_SYSCTRL_MAILBOX_H__ */

nit: we usually don't comment final endif

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> new file mode 100644
> index 000000000000..ce10924c5881
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef __XE_SYSCTRL_MAILBOX_TYPES_H__
> +#define __XE_SYSCTRL_MAILBOX_TYPES_H__
> +
> +#include <linux/types.h>
> +
> +struct xe_sysctrl_mailbox_mkhi_msg_hdr {
> +	__le32 data;
> +} __packed;
> +
> +struct xe_sysctrl_mailbox_app_msg_hdr {
> +	__le32 data;
> +} __packed;

as above definitions looks like HW/FW ABI, maybe we should put them into dedicated header in abi/ folder ?

> +

add proper kernel-doc comment for the struct itself
otherwise, members kernel-docs will not be recognized by the tool

> +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;
> +};
> +
> +#define SYSCTRL_MB_FRAME_SIZE			16
> +#define SYSCTRL_MB_MAX_FRAMES			64
> +#define SYSCTRL_MB_MAX_MESSAGE_SIZE		(SYSCTRL_MB_FRAME_SIZE * SYSCTRL_MB_MAX_FRAMES)
> +
> +#define SYSCTRL_MB_DEFAULT_TIMEOUT_MS		500

add XE_ prefix for above defs

> +
> +#endif /* __XE_SYSCTRL_MAILBOX_TYPES_H__ */

ditto

> 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..d4a362564925
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_types.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_SYSCTRL_TYPES_H_
> +#define _XE_SYSCTRL_TYPES_H_
> +
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct xe_mmio;
> +
> +/**
> + * struct xe_sysctrl - System Controller driver context
> + */
> +struct xe_sysctrl {
> +	/** @mmio: MMIO region for system control registers */
> +	struct xe_mmio *mmio;
> +
> +	/** @cmd_lock: Mutex protecting mailbox command operations */
> +	struct mutex cmd_lock;
> +
> +	/**
> +	 * @phase_bit: MKHI message boundary phase toggle bit
> +	 *
> +	 * Phase bit alternates between 0 and 1 for consecutive
> +	 * messages to help distinguish message boundaries.
> +	 */
> +	bool phase_bit;
> +};
> +
> +#endif /* _XE_SYSCTRL_TYPES_H_ */

ditto