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

[Anoop Vijay <anoop.c.vijay@intel.com>]

Hi Matt,

Thank you for your feedback on patch ordering. This has been addressed 
in v8. Patch series is reordered to follow logical initialization 
sequence as you suggested:

v8 Patch Order:
1. Types and device integration
2. Register definitions
3. Mailbox headers
4. Sysctrl Initialization
5. Mailbox Implementation
6. Platform enablement

Thanks,
Anoop

On 1/31/2026 1:29 AM, Matt Roper wrote:
> On Thu, Jan 29, 2026 at 04:10:45AM -0800, Anoop, Vijay wrote:
>> From: Anoop Vijay <anoop.c.vijay@intel.com>
>>
>> Add top-level System Controller initialization and cleanup:
>> - SOC remapper region configuration
>> - Mailbox initialization
>> - Resource cleanup on driver unload
>>
>> Signed-off-by: Anoop Vijay <anoop.c.vijay@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sysctrl.c | 80 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_sysctrl.h | 13 ++++++
>>   2 files changed, 93 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/xe_sysctrl.c b/drivers/gpu/drm/xe/xe_sysctrl.c
>> new file mode 100644
>> index 000000000000..430bccbdc3b9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_sysctrl.c
>> @@ -0,0 +1,80 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +#include <linux/device.h>
>> +#include <linux/mutex.h>
>> +
>> +#include "regs/xe_sysctrl_regs.h"
>> +#include "xe_device.h"
>> +#include "xe_mmio.h"
>> +#include "xe_printk.h"
>> +#include "xe_soc_remapper.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 xe_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.
>> + * 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 (!xe->soc_remapper.set_sysctrl_region)
>> +		return -ENODEV;
>> +
>> +	xe->soc_remapper.set_sysctrl_region(xe, SYSCTRL_MAILBOX_INDEX);
>> +
>> +	ret = devm_add_action_or_reset(xe->drm.dev, xe_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;
> 
> I assume the whole series is effectively "dead code" until we finally
> flip the flag on for some platforms in the last patch, but it's still
> somewhat confusing to be using sc->mmio in the previous patch, but not
> initializing it until this one. People usually review patches in series
> order, so while reading the previous patch, they may be wondering what
> the target of the MMIO operations is, which they won't find out until
> this later patch.  It can also cause confusion if someone is doing a
> bisect and inspecting the state of the code at some intermediate point.
> Is it possible to shuffle the order around so that general
> initialization comes earlier in the series, followed by code that builds
> upon the items initialized?
> 
> 
> Matt
> 
>> +
>> +	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..ee7826fe4c98
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_sysctrl.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2026 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_ */
>> -- 
>> 2.43.0
>>
>