Re: [PATCH v2 1/8] dt-bindings: mailbox: mediatek: Add GCE header file for MT8196

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

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@mediatek.com>
> > ---
> >  .../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
>