Re: [PATCH v9 4/6] drm/xe/sysctrl: Add System Controller initialization

[Riana Tauro <riana.tauro@intel.com>]

On 3/10/2026 11:53 PM, Anoop, Vijay wrote:
> From: Anoop Vijay <anoop.c.vijay@intel.com>
> 
> Add initialization infrastructure for System Controller subsystem:
> - xe_sysctrl_init() and cleanup handler
> - Integration into xe_device_probe()
> - Makefile build inclusion

The description should be a brief of what the patch is.
Mentioning 'makefile inclusions' is unnecessary

> 
> Initialization flow will check platform support via has_sysctrl flag and
> configure mailbox region through SOC remapper interface

punctuation..

> 
> Signed-off-by: Anoop Vijay <anoop.c.vijay@intel.com>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
> v8: (Matt, Michal, Shuicheng)
> - Fixed include order
> - Added VF check
> - Converted runtime checks to assertions
> - Added sc_to_xe() helper
> - Fixed kernel-doc syntax
> ---
>   drivers/gpu/drm/xe/Makefile     |  1 +
>   drivers/gpu/drm/xe/xe_device.c  |  5 ++
>   drivers/gpu/drm/xe/xe_sysctrl.c | 82 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_sysctrl.h | 21 +++++++++
>   4 files changed, 109 insertions(+)
>   create mode 100644 drivers/gpu/drm/xe/xe_sysctrl.c
>   create mode 100644 drivers/gpu/drm/xe/xe_sysctrl.h
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index f63fdf80055a..76a86818986a 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -123,6 +123,7 @@ xe-y += xe_bb.o \
>   	xe_step.o \
>   	xe_survivability_mode.o \
>   	xe_sync.o \
> +	xe_sysctrl.o \
>   	xe_tile.o \
>   	xe_tile_sysfs.o \
>   	xe_tlb_inval.o \
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index e77a3a3db73d..c70d4ae413a9 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -65,6 +65,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"
> @@ -985,6 +986,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_sysctrl.c b/drivers/gpu/drm/xe/xe_sysctrl.c
> new file mode 100644
> index 000000000000..2751fe25a6ff
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "regs/xe_sysctrl_regs.h"
> +#include "xe_assert.h"
> +#include "xe_device.h"
> +#include "xe_mmio.h"
> +#include "xe_printk.h"
> +#include "xe_soc_remapper.h"
> +#include "xe_sriov.h"
> +#include "xe_sysctrl.h"
> +#include "xe_sysctrl_mailbox.h"
> +#include "xe_sysctrl_types.h"
> +
> +/**
> + * DOC: System Controller (sysctrl)
> + *
> + * The System Controller (sysctrl) is an embedded microcontroller in Intel GPUs
> + * responsible for managing various low-level platform functions. Communication
> + * between the driver and the System Controller occurs via a mailbox interface,
> + * enabling the exchange of commands and responses.
> + *
> + * This module provides initialization routines and helper functions to interact
> + * with the System Controller through the mailbox.
> + */
> +static void sysctrl_fini(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	xe->soc_remapper.set_sysctrl_region(xe, 0);
> +}
> +
> +/**
> + * xe_sysctrl_init() - Initialize System Controller subsystem
> + * @xe: xe device instance
> + *
> + * Entry point for System Controller initialization, called from xe_device_probe.

How about suspend/resume?

> + * 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_tile *tile = xe_device_get_root_tile(xe);
> +	struct xe_sysctrl *sc = &xe->sc;
> +	int ret;
> +
> +	if (!xe->info.has_sysctrl)
> +		return 0;
> +
> +	if (IS_SRIOV_VF(xe))
> +		return 0;
> +
> +	xe_assert(xe, xe->soc_remapper.set_sysctrl_region);
> +
> +	xe->soc_remapper.set_sysctrl_region(xe, SYSCTRL_MAILBOX_INDEX);
> +
> +	ret = devm_add_action_or_reset(xe->drm.dev, sysctrl_fini, xe);
> +	if (ret)
> +		return ret;
> +
> +	sc->mmio = devm_kzalloc(xe->drm.dev, sizeof(*sc->mmio), GFP_KERNEL);
> +	if (!sc->mmio)
> +		return -ENOMEM;
> +
> +	xe_mmio_init(sc->mmio, tile, tile->mmio.regs, tile->mmio.regs_size);
> +	sc->mmio->adj_offset = SYSCTRL_BASE;
> +	sc->mmio->adj_limit = U32_MAX;
> +
> +	ret = drmm_mutex_init(&xe->drm, &sc->cmd_lock);

Let's use devm or drmm. Not a mix of both.
Sc is specific to device so devm would be appropriate.

Thanks
Riana

> +	if (ret)
> +		return ret;
> +
> +	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..d5d8735038ae
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_sysctrl.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_SYSCTRL_H_
> +#define _XE_SYSCTRL_H_
> +
> +#include <linux/container_of.h>
> +
> +#include "xe_device_types.h"
> +#include "xe_sysctrl_types.h"
> +
> +static inline struct xe_device *sc_to_xe(struct xe_sysctrl *sc)
> +{
> +	return container_of(sc, struct xe_device, sc);
> +}
> +
> +int xe_sysctrl_init(struct xe_device *xe);
> +
> +#endif