Re: [PATCH v1 2/2] firmware: arm_scmi: Add MediaTek TinySYS SCMI Protocol support

[Peng Fan <peng.fan@oss.nxp.com>]

On Mon, Jun 23, 2025 at 02:01:36PM +0200, AngeloGioacchino Del Regno wrote:
>Add a driver for the SCMI protocol extensions for MediaTek TinySYS.
>This is used to communicate with various remote processors in some
>MediaTek SoCs, which mainly handle power management related tasks.
>
>Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>---
> drivers/firmware/arm_scmi/Kconfig             |   1 +
> drivers/firmware/arm_scmi/Makefile            |   1 +
> .../arm_scmi/vendors/mediatek/Kconfig         |  16 +
> .../arm_scmi/vendors/mediatek/Makefile        |   2 +
> .../arm_scmi/vendors/mediatek/mtk-tinysys.c   | 344 ++++++++++++++++++
> include/linux/scmi_mtk_protocol.h             |  62 ++++
> 6 files changed, 426 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/vendors/mediatek/Kconfig
> create mode 100644 drivers/firmware/arm_scmi/vendors/mediatek/Makefile
> create mode 100644 drivers/firmware/arm_scmi/vendors/mediatek/mtk-tinysys.c
> create mode 100644 include/linux/scmi_mtk_protocol.h
>
>diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
>index e3fb36825978..baadf4f7fef6 100644
>--- a/drivers/firmware/arm_scmi/Kconfig
>+++ b/drivers/firmware/arm_scmi/Kconfig
>@@ -84,6 +84,7 @@ config ARM_SCMI_QUIRKS
> 
> source "drivers/firmware/arm_scmi/transports/Kconfig"
> source "drivers/firmware/arm_scmi/vendors/imx/Kconfig"
>+source "drivers/firmware/arm_scmi/vendors/mediatek/Kconfig"
> 
> endif #ARM_SCMI_PROTOCOL
> 
>diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
>index 780cd62b2f78..d1b4ec16b8bc 100644
>--- a/drivers/firmware/arm_scmi/Makefile
>+++ b/drivers/firmware/arm_scmi/Makefile
>@@ -13,6 +13,7 @@ scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> 
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += transports/
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/imx/
>+obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/mediatek/
> 
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>diff --git a/drivers/firmware/arm_scmi/vendors/mediatek/Kconfig b/drivers/firmware/arm_scmi/vendors/mediatek/Kconfig
>new file mode 100644
>index 000000000000..8facdcd3819f
>--- /dev/null
>+++ b/drivers/firmware/arm_scmi/vendors/mediatek/Kconfig
>@@ -0,0 +1,16 @@
>+# SPDX-License-Identifier: GPL-2.0-only
>+menu "ARM SCMI MediaTek Vendor Protocols"
>+
>+config MTK_SCMI_TINYSYS
>+	tristate "MediaTek SCMI TinySYS Extension"
>+	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)

Should dependency to ARCH_MEDIATEK be added, as below
ARM_SCMI_PROTOCOL & ARCH_MEDIATEK ?

>+	default y if ARCH_MEDIATEK
>+	help
>+	  This enables communication with the MediaTek TinySYS MCU
>+	  to control the power status of various SoC sub-devices
>+	  other than passing other messages for initialization.
>+
>+	  To compile this driver as a module, choose M here: the
>+	  module will be called mtk-tinysys.
>+
>+endmenu
>diff --git a/drivers/firmware/arm_scmi/vendors/mediatek/Makefile b/drivers/firmware/arm_scmi/vendors/mediatek/Makefile
>new file mode 100644
>index 000000000000..dc1ff63c3b69
>--- /dev/null
>+++ b/drivers/firmware/arm_scmi/vendors/mediatek/Makefile
>@@ -0,0 +1,2 @@
>+# SPDX-License-Identifier: GPL-2.0-only
>+obj-$(CONFIG_MTK_SCMI_TINYSYS) += mtk-tinysys.o
>diff --git a/drivers/firmware/arm_scmi/vendors/mediatek/mtk-tinysys.c b/drivers/firmware/arm_scmi/vendors/mediatek/mtk-tinysys.c
>new file mode 100644
>index 000000000000..baeb36493952
>--- /dev/null
>+++ b/drivers/firmware/arm_scmi/vendors/mediatek/mtk-tinysys.c
>@@ -0,0 +1,344 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * System Control and Management Interface (SCMI) MediaTek TinySYS Protocol
>+ *
>+ * Copyright (c) 2021 MediaTek Inc.
>+ * Copyright (c) 2025 Collabora Ltd
>+ *                    AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>+ */
>+
>+#define pr_fmt(fmt) "SCMI Notifications TinySYS - " fmt
>+
>+#include <linux/bits.h>
>+#include <linux/io.h>
>+#include <linux/module.h>
>+#include <linux/of.h>
>+#include <linux/platform_device.h>

This header is not needed, since no user here.
I just see i.MX also includes this header, but could be dropped.

>+#include <linux/scmi_protocol.h>
>+#include <linux/scmi_mtk_protocol.h>
>+
>+#include "../../protocols.h"
>+#include "../../notify.h"
>+
>+#define SCMI_MTK_CMD_SSPM_QUERY_ALIVE		0xdead
>+
>+enum scmi_mtk_tinysys_protocol_cmd {
>+	MTK_TINYSYS_COMMON_SET = 0x3,
>+	MTK_TINYSYS_COMMON_GET = 0x4,
>+	MTK_TINYSYS_POWER_STATE_NOTIFY = 0x5,
>+	MTK_TINYSYS_SLBC_CTRL = 0x6,
>+};
>+
>+struct scmi_mtk_tinysys_common_get_payld {
>+	__le32 rsvd;
>+	__le32 param[SCMI_MTK_MSG_COMMON_REPLY_BYTES];
>+};
>+
>+struct scmi_mtk_tinysys_common_set_payld {
>+	__le32 rsvd;
>+	__le32 ctrl_id;
>+	__le32 param[SCMI_MTK_MSG_COMMON_PARAM_BYTES];
>+};
>+
>+struct scmi_mtk_tinysys_slbc_payld {
>+	__le32 rsvd;
>+	__le32 cmd;
>+	__le32 arg[SCMI_MTK_MSG_SLBC_PARAM_BYTES];
>+};
>+
>+struct scmi_mtk_tinysys_pwrst_notify {
>+	__le32 rsvd;

Not sure why all upper structure has 'rsvd', meaning reserved?

>+	__le32 fid;
>+	__le32 enable;
>+};
>+
>+struct scmi_mtk_tinysys_notify_payld {
>+	__le32 fid;
>+	__le32 param[SCMI_MTK_MSG_NOTIF_ST_BYTES];
>+};
>+
>+struct scmi_mtk_tinysys_protocol_attributes {
>+	__le32 attributes;
>+};
>+
>+struct scmi_mtk_tinysys_info {
>+	int num_domains;
>+};
>+
>+static int scmi_mtk_tinysys_attributes_get(const struct scmi_protocol_handle *ph,
>+					   struct scmi_mtk_tinysys_info *tinfo)
>+{
>+	struct scmi_mtk_tinysys_protocol_attributes *attr;
>+	struct scmi_xfer *t;
>+	int ret;
>+
>+	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t);
>+	if (ret)
>+		return ret;
>+
>+	attr = t->rx.buf;
>+
>+	ret = ph->xops->do_xfer(ph, t);
>+	if (!ret) {
>+		attr->attributes = get_unaligned_le32(t->rx.buf);
>+		tinfo->num_domains = attr->attributes;

Not sure the spec use whole 32bits for num_domains, if not, better
use le32_get_bits to the expect fields

>+	}
>+
>+	ph->xops->xfer_put(ph, t);
>+
>+	return ret;
>+}
>+
>+static int scmi_mtk_tinysys_get_num_sources(const struct scmi_protocol_handle *ph)
>+{
>+	struct scmi_mtk_tinysys_info *tinfo = ph->get_priv(ph);
>+
>+	if (!tinfo)
>+		return -EINVAL;
>+
>+	return tinfo->num_domains;
>+}
>+
>+static int scmi_mtk_tinysys_set_notify_enabled(const struct scmi_protocol_handle *ph,
>+					       u8 evt_id, u32 src_id, bool enable)
>+{
>+	struct scmi_mtk_tinysys_pwrst_notify *pwrst_notify;
>+	struct scmi_xfer *t;
>+	int ret;
>+
>+	/* There's only one possible event for now */
>+	if (evt_id != 0)

Use 'evt_id != SCMI_EVENT_MTK_TINYSYS_NOTIFIER' ?

>+		return -EINVAL;
>+
...
>+#endif
>-- 
>2.49.0
>

Regards,
Peng