Hi Krzysztof,
Thanks for the reviews.
On Wed, 2024-12-11 at 10:37 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On Wed, Dec 11, 2024 at 11:22:49AM +0800, Jason-JH.Lin wrote:
> > Add the Global Command Engine (GCE) header file to define the GCE
> > thread priority, GCE subsys ID and GCE events for MT8196.
>
> This we see from the diff. What we do not see is why priority is a
> binding. Looking briefly at existing code: it is not a binding, there
> is
> no driver user.
>
This priority value is used to configure the priority level for each
GCE hardware thread, so it is a necessary hardware attribute.
It's hard to find where the priority is used in existing driver code
because we parsed it from DTS.
It is used in all mediaTeks' DTS using the GCE.
For example, in mt8195.dts:
vdosys0: syscon@1c01a000 {
compatible = "mediatek,mt8195-vdosys0", "mediatek,mt8195-mmsys",
"syscon";
reg = <0 0x1c01a000 0 0x1000>;
mboxes = <&gce0 0 CMDQ_THR_PRIO_4>;
#clock-cells = <1>;
mediatek,gce-client-reg = <&gce0 SUBSYS_1c01XXXX 0xa000 0x1000>;
}
CMDQ driver(mtk-cmdq-mailbox.c) will get the args parsed from mboxes
property in cmdq_xlate() and then it will store CMDQ_THR_PRIO_4 to the
specific thread structure.
The user of CMDQ driver will send command to CMDQ driver by
cmdq_mbox_send_data(), and this priority setting will be configured to
GCE hardware thread.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> > ---
> > .../dt-bindings/mailbox/mediatek,mt8196-gce.h | 1439
> > +++++++++++++++++
> > 1 file changed, 1439 insertions(+)
> > create mode 100644 include/dt-bindings/mailbox/mediatek,mt8196-
> > gce.h
> >
> > diff --git a/include/dt-bindings/mailbox/mediatek,mt8196-gce.h
> > b/include/dt-bindings/mailbox/mediatek,mt8196-gce.h
> > new file mode 100644
> > index 000000000000..860d69100157
> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/mediatek,mt8196-gce.h
> > @@ -0,0 +1,1439 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + *
> > + */
> > +
> > +#ifndef _DT_BINDINGS_GCE_MT8196_H
> > +#define _DT_BINDINGS_GCE_MT8196_H
> > +
> > +/* GCE thread priority */
> > +#define CMDQ_THR_PRIO_LOWEST 0
> > +#define CMDQ_THR_PRIO_1 1
> > +#define CMDQ_THR_PRIO_2 2
> > +#define CMDQ_THR_PRIO_3 3
> > +#define CMDQ_THR_PRIO_4 4
> > +#define CMDQ_THR_PRIO_5 5
> > +#define CMDQ_THR_PRIO_6 6
> > +#define CMDQ_THR_PRIO_HIGHEST 7
> > +
> > +/* GCE subsys table */
> > +#define SUBSYS_1300XXXX 0
> > +#define SUBSYS_1400XXXX 1
> > +#define SUBSYS_1401XXXX 2
> > +#define SUBSYS_1402XXXX 3
> > +#define SUBSYS_1502XXXX 4
> > +#define SUBSYS_1880XXXX 5
> > +#define SUBSYS_1881XXXX 6
> > +#define SUBSYS_1882XXXX 7
> > +#define SUBSYS_1883XXXX 8
> > +#define SUBSYS_1884XXXX 9
> > +#define SUBSYS_1000XXXX 10
> > +#define SUBSYS_1001XXXX 11
> > +#define SUBSYS_1002XXXX 12
> > +#define SUBSYS_1003XXXX 13
> > +#define SUBSYS_1004XXXX 14
> > +#define SUBSYS_1005XXXX 15
> > +#define SUBSYS_1020XXXX 16
> > +#define SUBSYS_1028XXXX 17
> > +#define SUBSYS_1700XXXX 18
> > +#define SUBSYS_1701XXXX 19
> > +#define SUBSYS_1702XXXX 20
> > +#define SUBSYS_1703XXXX 21
> > +#define SUBSYS_1800XXXX 22
> > +#define SUBSYS_1801XXXX 23
> > +#define SUBSYS_1802XXXX 24
> > +#define SUBSYS_1804XXXX 25
> > +#define SUBSYS_1805XXXX 26
> > +#define SUBSYS_1808XXXX 27
> > +#define SUBSYS_180aXXXX 28
> > +#define SUBSYS_180bXXXX 29
> > +#define SUBSYS_NO_SUPPORT 99
> > +
> > +/*
> > + * GCE General Purpose Register (GPR) support
> > + * Leave note for scenario usage here
> > + */
> > +/* GCE: write mask */
>
> That's a definite no-go. Register masks are not bindings.
>
I'm sorry to the confusion.
These defines are the index of GCE General Purpose Register for
generating instructions, they are not register masks.
The comment "/* GCE: write mask */" is briefly describe that the usage
of GCE_GPR_R0 and GCE_GPR_R01 is used to store the register mask when
GCE executing the WRITE instruction. And it can also store the register
mask of POLL and READ instruction.
I will add more words to make this comment clearer, like this:
/*GCE: store the mask of instruction */
Regards,
Jason-JH.Lin
> > +#define GCE_GPR_R00 0x0
> > +#define GCE_GPR_R01 0x1
>
> Best regards,
> Krzysztof
>
