* [PATCH 0/3] MediaTek mt6572 GCE support
@ 2026-07-28 9:42 Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 1/3] dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572 Roman Vivchar via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Roman Vivchar via B4 Relay @ 2026-07-28 9:42 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Houlong Wei
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek,
Roman Vivchar
Yes I know this one is very different from basics like previous pwrap
and efuse...
This patch series adds support for the Global Command Engine found in
the mt6572 SoC.
Notably, the patch 2 introduces legacy ISA format, which is used on
mt6572, 6582 and 6592 SoCs. mt6577 doesn't have the GCE at all, mt6589
seems to have new-ish ISA, and mt6595 is much more close to 6795/8173.
Patch 3 adds mt6572 platform data and legacy_isa forwarding to the
instruction JIT.
This format was reverse engineered from the Android HAL and uses 22-bit
offset with 2-bit subsystem ID, unlike the newer 16-bit offset and 8-bit
subsystem ID.
The mt6572 HAL includes workarounds for WFE and EOC instructions, emitting
another WFE as barrier at WFE, and JUMP +8 at EOC.
mt6582 and 6592 HAL dropped WFE barrier, but still have JUMP +8.
This sounds very much like some kind of software mitigations for hardware
bugs, but during testing on 6572 nothing bad happened so far. These
additional instructions are not included in the patches.
Note that while GCE can be used for e.g. mt6592 UFOE (which is at
0x14013000, so outside of the u16 range), currently the driver still uses
u16 for the offset, because I don't have any 6592 device.
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
Roman Vivchar (3):
dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572
soc: mediatek: cmdq-helper: add legacy GCE ISA support
mailbox: mtk-cmdq: add mt6572 support
.../bindings/mailbox/mediatek,gce-mailbox.yaml | 1 +
drivers/mailbox/mtk-cmdq-mailbox.c | 11 +++++
drivers/soc/mediatek/mtk-cmdq-helper.c | 55 +++++++++++++++-------
include/dt-bindings/gce/mediatek,mt6572-gce.h | 52 ++++++++++++++++++++
include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
5 files changed, 103 insertions(+), 17 deletions(-)
---
base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
change-id: 20260727-6572-gce-6a571ef4af09
Best regards,
--
Roman Vivchar <rva333@protonmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572
2026-07-28 9:42 [PATCH 0/3] MediaTek mt6572 GCE support Roman Vivchar via B4 Relay
@ 2026-07-28 9:42 ` Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support Roman Vivchar via B4 Relay
2 siblings, 0 replies; 6+ messages in thread
From: Roman Vivchar via B4 Relay @ 2026-07-28 9:42 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Houlong Wei
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek,
Roman Vivchar
From: Roman Vivchar <rva333@protonmail.com>
Add a compatible string and a header for the GCE constants.
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
.../bindings/mailbox/mediatek,gce-mailbox.yaml | 1 +
include/dt-bindings/gce/mediatek,mt6572-gce.h | 52 ++++++++++++++++++++++
2 files changed, 53 insertions(+)
diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
index 587126d03fc6..d6e93010bb94 100644
--- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
@@ -18,6 +18,7 @@ properties:
compatible:
oneOf:
- enum:
+ - mediatek,mt6572-gce
- mediatek,mt6779-gce
- mediatek,mt8173-gce
- mediatek,mt8183-gce
diff --git a/include/dt-bindings/gce/mediatek,mt6572-gce.h b/include/dt-bindings/gce/mediatek,mt6572-gce.h
new file mode 100644
index 000000000000..e6d949d76f88
--- /dev/null
+++ b/include/dt-bindings/gce/mediatek,mt6572-gce.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2026 Roman Vivchar.
+ */
+
+#ifndef _DT_BINDINGS_GCE_MT6572_H
+#define _DT_BINDINGS_GCE_MT6572_H
+
+/* GCE HW thread priority */
+#define CMDQ_THR_PRIO_NORMAL 0
+#define CMDQ_THR_PRIO_HIGHEST 1
+
+/* GCE SUBSYS */
+#define SUBSYS_14XXXXXX 0
+#define SUBSYS_15XXXXXX 1
+
+/* GCE HW EVENT */
+#define CMDQ_EVENT_MUTEX0 0
+#define CMDQ_EVENT_MUTEX1 1
+#define CMDQ_EVENT_MUTEX2 2
+#define CMDQ_EVENT_MUTEX3 3
+#define CMDQ_EVENT_MUTEX4 4
+#define CMDQ_EVENT_MUTEX5 5
+#define CMDQ_EVENT_MUTEX6 6
+#define CMDQ_EVENT_MUTEX7 7
+
+#define CMDQ_EVENT_DISP_WDMA_EOF 12
+#define CMDQ_EVENT_DISP_RDMA_EOF 13
+#define CMDQ_EVENT_DISP_BLS_EOF 14
+#define CMDQ_EVENT_DISP_COLOR_EOF 15
+#define CMDQ_EVENT_DISP_OVL_EOF 16
+#define CMDQ_EVENT_MDP_TDSHP_EOF 17
+#define CMDQ_EVENT_MDP_RSZ1_EOF 18
+#define CMDQ_EVENT_MDP_RSZ0_EOF 19
+#define CMDQ_EVENT_MDP_RDMA_EOF 20
+#define CMDQ_EVENT_MDP_WDMA_EOF 21
+#define CMDQ_EVENT_MDP_WROT_EOF 22
+
+#define CMDQ_EVENT_MDP_WROT_SOF 24
+#define CMDQ_EVENT_MDP_RSZ0_SOF 25
+#define CMDQ_EVENT_MDP_RSZ1_SOF 26
+#define CMDQ_EVENT_DISP_OVL_SOF 27
+#define CMDQ_EVENT_MDP_WDMA_SOF 28
+#define CMDQ_EVENT_MDP_RDMA_SOF 29
+#define CMDQ_EVENT_DISP_WDMA_SOF 30
+#define CMDQ_EVENT_DISP_COLOR_SOF 31
+#define CMDQ_EVENT_MDP_TDSHP_SOF 32
+#define CMDQ_EVENT_DISP_BLS_SOF 33
+#define CMDQ_EVENT_DISP_RDMA_SOF 34
+#define CMDQ_EVENT_CAM_MDP 35
+
+#endif
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support
2026-07-28 9:42 [PATCH 0/3] MediaTek mt6572 GCE support Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 1/3] dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572 Roman Vivchar via B4 Relay
@ 2026-07-28 9:42 ` Roman Vivchar via B4 Relay
2026-07-28 9:59 ` sashiko-bot
2026-07-28 9:42 ` [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support Roman Vivchar via B4 Relay
2 siblings, 1 reply; 6+ messages in thread
From: Roman Vivchar via B4 Relay @ 2026-07-28 9:42 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Houlong Wei
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek,
Roman Vivchar
From: Roman Vivchar <rva333@protonmail.com>
Some SoCs, such as mt6572, have different WRITE and POLL instruction
encoding. Instead of 16-bit offset and 8-bit subsystem ID, old GCEs have
22-bit offset and 2-bit subsystem ID.
Extend cmdq_instruction struct with additional union and struct to hold
the legacy format of instruction encoding. Add legacy_isa to select
legacy GCE ISA when writing instruction. Finally, add special handing
for legacy_isa cases.
While mt6572 GCE supports 22-bit offsets, the parameter type for offset
remains 16 bit, because all mt6572 GCE mmsys consumers are in the 0xffff
range.
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 55 ++++++++++++++++++++++----------
include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
2 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index f8ee6c9ade89..094732405080 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -31,18 +31,27 @@ struct cmdq_instruction {
};
};
union {
- u16 offset;
- u16 event;
- u16 reg_dst;
- };
- union {
- u8 subsys;
struct {
- u8 sop:5;
- u8 arg_c_t:1;
- u8 src_t:1;
- u8 dst_t:1;
- };
+ union {
+ u16 offset;
+ u16 event;
+ u16 reg_dst;
+ };
+ union {
+ u8 subsys;
+ struct {
+ u8 sop:5;
+ u8 arg_c_t:1;
+ u8 src_t:1;
+ u8 dst_t:1;
+ };
+ };
+ } __packed;
+
+ struct {
+ u32 offset_legacy:22;
+ u32 subsys_legacy:2;
+ } __packed;
};
u8 op;
};
@@ -219,10 +228,16 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
{
struct cmdq_instruction inst = {
.op = CMDQ_CODE_WRITE,
- .value = value,
- .offset = offset,
- .subsys = subsys
+ .value = value
};
+
+ if (pkt->priv.legacy_isa) {
+ inst.offset_legacy = offset;
+ inst.subsys_legacy = subsys;
+ } else {
+ inst.offset = offset;
+ inst.subsys = subsys;
+ }
return cmdq_pkt_append_command(pkt, inst);
}
EXPORT_SYMBOL(cmdq_pkt_write);
@@ -459,10 +474,16 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
{
struct cmdq_instruction inst = {
.op = CMDQ_CODE_POLL,
- .value = value,
- .offset = offset,
- .subsys = subsys
+ .value = value
};
+
+ if (pkt->priv.legacy_isa) {
+ inst.offset_legacy = offset;
+ inst.subsys_legacy = subsys;
+ } else {
+ inst.offset = offset;
+ inst.subsys = subsys;
+ }
return cmdq_pkt_append_command(pkt, inst);
}
EXPORT_SYMBOL(cmdq_pkt_poll);
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index 07c1bfbdb8c4..9fcebc4ca864 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -72,6 +72,7 @@ struct cmdq_cb_data {
struct cmdq_mbox_priv {
u8 shift_pa;
+ bool legacy_isa;
dma_addr_t mminfra_offset;
};
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support
2026-07-28 9:42 [PATCH 0/3] MediaTek mt6572 GCE support Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 1/3] dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572 Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support Roman Vivchar via B4 Relay
@ 2026-07-28 9:42 ` Roman Vivchar via B4 Relay
2026-07-28 9:56 ` sashiko-bot
2 siblings, 1 reply; 6+ messages in thread
From: Roman Vivchar via B4 Relay @ 2026-07-28 9:42 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Houlong Wei
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-mediatek,
Roman Vivchar
From: Roman Vivchar <rva333@protonmail.com>
Add legacy_isa field forwarding to the cmdq helper and a platform data
for the mt6572 GCE.
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
drivers/mailbox/mtk-cmdq-mailbox.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index e523c84b4808..5e7bc52baaa1 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -99,6 +99,7 @@ struct gce_plat {
bool control_by_sw;
bool sw_ddr_en;
bool gce_vm;
+ bool legacy_isa;
u32 gce_num;
};
@@ -119,6 +120,7 @@ void cmdq_get_mbox_priv(struct mbox_chan *chan, struct cmdq_mbox_priv *priv)
struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
priv->shift_pa = cmdq->pdata->shift;
+ priv->legacy_isa = cmdq->pdata->legacy_isa;
priv->mminfra_offset = cmdq->pdata->mminfra_offset;
}
EXPORT_SYMBOL(cmdq_get_mbox_priv);
@@ -787,6 +789,14 @@ static const struct dev_pm_ops cmdq_pm_ops = {
cmdq_runtime_resume, NULL)
};
+static const struct gce_plat gce_plat_mt6572 = {
+ .thread_nr = 14,
+ .shift = 0,
+ .control_by_sw = false,
+ .legacy_isa = true,
+ .gce_num = 1
+};
+
static const struct gce_plat gce_plat_mt6779 = {
.thread_nr = 24,
.shift = 3,
@@ -848,6 +858,7 @@ static const struct gce_plat gce_plat_mt8196 = {
};
static const struct of_device_id cmdq_of_ids[] = {
+ {.compatible = "mediatek,mt6572-gce", .data = (void *)&gce_plat_mt6572},
{.compatible = "mediatek,mt6779-gce", .data = (void *)&gce_plat_mt6779},
{.compatible = "mediatek,mt8173-gce", .data = (void *)&gce_plat_mt8173},
{.compatible = "mediatek,mt8183-gce", .data = (void *)&gce_plat_mt8183},
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support
2026-07-28 9:42 ` [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support Roman Vivchar via B4 Relay
@ 2026-07-28 9:56 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-07-28 9:56 UTC (permalink / raw)
To: Roman Vivchar; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Missing `mminfra_offset` translation in `cmdq_task_handle_error()` causes the GCE hardware to jump to a garbage physical address on error recovery.
- [High] Unbalanced `of_clk_get()` for multi-GCE platforms leads to a leak of clock references in error paths and during unbind.
- [High] Potential NULL pointer dereference in the IRQ handler due to uninitialized `thread->chan`.
--
--- Patch [3]: [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support ---
commit 125c66f031e1e0c9602b803f39b71e1fb66eb336
Author: Roman Vivchar <rva333@protonmail.com>
mailbox: mtk-cmdq: add mt6572 support
Add legacy_isa field forwarding to the cmdq helper and a platform data
for the mt6572 GCE.
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index e523c84b48088..5e7bc52baaa13 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -99,6 +99,7 @@ struct gce_plat {
> bool control_by_sw;
> bool sw_ddr_en;
> bool gce_vm;
> + bool legacy_isa;
> u32 gce_num;
> };
>
> @@ -119,6 +120,7 @@ void cmdq_get_mbox_priv(struct mbox_chan *chan, struct cmdq_mbox_priv *priv)
> struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
>
> priv->shift_pa = cmdq->pdata->shift;
> + priv->legacy_isa = cmdq->pdata->legacy_isa;
> priv->mminfra_offset = cmdq->pdata->mminfra_offset;
> }
> EXPORT_SYMBOL(cmdq_get_mbox_priv);
> @@ -787,6 +789,14 @@ static const struct dev_pm_ops cmdq_pm_ops = {
> cmdq_runtime_resume, NULL)
> };
>
> +static const struct gce_plat gce_plat_mt6572 = {
> + .thread_nr = 14,
> + .shift = 0,
> + .control_by_sw = false,
> + .legacy_isa = true,
> + .gce_num = 1
> +};
> +
> static const struct gce_plat gce_plat_mt6779 = {
> .thread_nr = 24,
> .shift = 3,
> @@ -848,6 +858,7 @@ static const struct gce_plat gce_plat_mt8196 = {
> };
>
> static const struct of_device_id cmdq_of_ids[] = {
> + {.compatible = "mediatek,mt6572-gce", .data = (void *)&gce_plat_mt6572},
> {.compatible = "mediatek,mt6779-gce", .data = (void *)&gce_plat_mt6779},
> {.compatible = "mediatek,mt8173-gce", .data = (void *)&gce_plat_mt8173},
> {.compatible = "mediatek,mt8183-gce", .data = (void *)&gce_plat_mt8183},
[Severity: High]
This isn't a bug introduced by this patch, but there is an unbalanced
of_clk_get() for multi-GCE platforms that leads to a leak of clock references.
When cmdq->pdata->gce_num > 1, additional clocks are fetched in
cmdq_get_clocks():
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_get_clocks() {
...
for_each_child_of_node_scoped(parent, node) {
...
clks->clk = of_clk_get(node, 0);
if (IS_ERR(clks->clk))
return dev_err_probe(dev, PTR_ERR(clks->clk),
"failed to get gce%d clock\n", alias_id);
}
...
}
Unlike the first clock which uses devm_clk_get(), of_clk_get() returns an
unmanaged clock reference. If the loop encounters an error on a subsequent
clock, does this return immediately and leak the references for the clocks
already successfully fetched?
Furthermore, during driver unbind, the driver unprepares the clocks but never
calls clk_put() to release them:
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_remove() {
...
clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks);
}
Should clk_put() be called on these unmanaged clock references in error paths
and during unbind to prevent a permanent resource leak?
[Severity: High]
This is a pre-existing issue, but is there a potential NULL pointer dereference
in the IRQ handler due to an uninitialized thread->chan?
The driver unconditionally registers cmdq_irq_handler() with IRQF_SHARED
during cmdq_probe(). When an interrupt is delivered, the handler iterates over
the active threads and attempts to lock the channel:
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_irq_handler() {
...
for_each_clear_bit(bit, &irq_status, cmdq->pdata->thread_nr) {
struct cmdq_thread *thread = &cmdq->thread[bit];
spin_lock_irqsave(&thread->chan->lock, flags);
cmdq_thread_irq_handler(cmdq, thread);
spin_unlock_irqrestore(&thread->chan->lock, flags);
}
...
}
However, thread->chan is only initialized dynamically when a client requests
the channel via cmdq_xlate():
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_xlate() {
...
thread->chan = &mbox->chans[ind];
...
}
If a shared interrupt line fires, or a spurious interrupt arrives, or the
bootloader left the hardware in an interrupting state before the client has
requested the mailbox, could the handler dereference a NULL thread->chan here
and cause a kernel oops?
[Severity: Critical]
This isn't a bug introduced by this patch, but does cmdq_task_handle_error()
miss applying the mminfra_offset translation, potentially causing the GCE
hardware to jump to a garbage physical address on error recovery?
When an error occurs and the driver attempts to skip the failed task, it
evaluates the next task's execution address by simply right-shifting its
physical address and writes this directly to the hardware:
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_task_handle_error() {
...
next_task = list_first_entry_or_null(&thread->task_busy_list,
struct cmdq_task, list_entry);
if (next_task)
writel(next_task->pa_base >> cmdq->pdata->shift,
thread->base + CMDQ_THR_CURR_ADDR);
...
}
This ignores the cmdq->pdata->mminfra_offset adjustment which is required for
correct conversion, as handled properly elsewhere in the driver:
drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_convert_gce_addr() {
...
return (addr + pdata->mminfra_offset) >> pdata->shift;
}
On SoCs with a non-zero memory infrastructure offset (such as MT8196), will the
hardware receive a severely truncated and incorrect address, leading to memory
corruption or a catastrophic system hang when the thread resumes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-6572-gce-v1-0-750bba32689e@protonmail.com?part=3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support
2026-07-28 9:42 ` [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support Roman Vivchar via B4 Relay
@ 2026-07-28 9:59 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-07-28 9:59 UTC (permalink / raw)
To: Roman Vivchar; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `legacy_isa` feature is completely non-functional because the driver fails to add the `mt6572` compatible string to `cmdq_of_ids` and `cmdq_get_mbox_priv()` is never updated to initialize the `legacy_isa` field. As a result, `pkt->priv.legacy_isa` always defaults to `false` (as packets are zero-allocated by callers), rendering the new legacy instruction support dead code.
- [High] The bitfield layout for `offset_legacy` and `subsys_legacy` inside the `__packed` struct relies on compiler-dependent little-endian bitfield ordering, which breaks on big-endian kernels.
- [High] `cmdq_pkt_poll_addr()` manually constructs a `CMDQ_CODE_POLL` instruction using standard hardware bitfields (like `sop` and `dst_t`), bypassing the `legacy_isa` encoding checks. This will emit corrupted instructions on MT6572.
--
--- Patch [2]: [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support ---
commit 79388ed6a8bb4f65925fd916d2fe04a8f2d226d6
Author: Roman Vivchar <rva333@protonmail.com>
> soc: mediatek: cmdq-helper: add legacy GCE ISA support
>
> Some SoCs, such as mt6572, have different WRITE and POLL instruction
> encoding. Instead of 16-bit offset and 8-bit subsystem ID, old GCEs have
> 22-bit offset and 2-bit subsystem ID.
>
> Extend cmdq_instruction struct with additional union and struct to hold
> the legacy format of instruction encoding. Add legacy_isa to select
> legacy GCE ISA when writing instruction. Finally, add special handing
> for legacy_isa cases.
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index f8ee6c9ade898..0947324050805 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -31,18 +31,27 @@ struct cmdq_instruction {
> [ ... ]
> + struct {
> + u32 offset_legacy:22;
> + u32 subsys_legacy:2;
> + } __packed;
[Severity: High]
Could this bitfield layout break on big-endian kernels?
C bitfields are packed starting from the least significant bit on
little-endian architectures, but from the most significant bit on big-endian
architectures. If this is compiled on a big-endian kernel, the bit layout
will be inverted and the hardware might receive malformed instruction words.
Would it be better to use explicit bitwise shifts and masks, or an explicit
little-endian bitfield declaration for hardware-facing structures?
> [ ... ]
> @@ -459,10 +474,16 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
> {
> struct cmdq_instruction inst = {
> .op = CMDQ_CODE_POLL,
> - .value = value,
> - .offset = offset,
> - .subsys = subsys
> + .value = value
> };
> +
> + if (pkt->priv.legacy_isa) {
> + inst.offset_legacy = offset;
> + inst.subsys_legacy = subsys;
> + } else {
> + inst.offset = offset;
> + inst.subsys = subsys;
> + }
> return cmdq_pkt_append_command(pkt, inst);
> }
[Severity: High]
Does cmdq_pkt_poll_addr() also need an update for the new legacy format?
While cmdq_pkt_poll() checks for legacy_isa here, cmdq_pkt_poll_addr()
appears to still manually construct a poll instruction using standard hardware
bitfields like sop and dst_t.
If cmdq_pkt_poll_addr() is called on older hardware like mt6572, these bits
might be incorrectly reinterpreted by the hardware as random legacy offsets,
potentially causing hardware faults. Should cmdq_pkt_poll_addr() explicitly
return -EOPNOTSUPP if legacy_isa is active?
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 07c1bfbdb8c46..9fcebc4ca8645 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -72,6 +72,7 @@ struct cmdq_cb_data {
>
> struct cmdq_mbox_priv {
> u8 shift_pa;
> + bool legacy_isa;
[Severity: High]
Is the legacy_isa flag ever initialized to true?
Looking at the driver, it appears the mt6572 compatible string is missing
from cmdq_of_ids, and cmdq_get_mbox_priv() is not updated to initialize
this field.
Since packets are zero-allocated by callers, this flag will always default
to false, meaning the new legacy instruction branches will not be executed
and the feature remains non-functional.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-6572-gce-v1-0-750bba32689e@protonmail.com?part=2
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-28 9:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-28 9:42 [PATCH 0/3] MediaTek mt6572 GCE support Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 1/3] dt-bindings: mailbox: mediatek,gce-mailbox: add mt6572 Roman Vivchar via B4 Relay
2026-07-28 9:42 ` [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support Roman Vivchar via B4 Relay
2026-07-28 9:59 ` sashiko-bot
2026-07-28 9:42 ` [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support Roman Vivchar via B4 Relay
2026-07-28 9:56 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox