From: "Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>
To: "jassisinghbrar@gmail.com" <jassisinghbrar@gmail.com>,
"wenst@chromium.org" <wenst@chromium.org>
Cc: Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@mediatek.com>,
"robh@kernel.org" <robh@kernel.org>,
"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
"simona@ffwll.ch" <simona@ffwll.ch>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"conor+dt@kernel.org" <conor+dt@kernel.org>,
"linux-mediatek@lists.infradead.org"
<linux-mediatek@lists.infradead.org>,
"chunkuang.hu@kernel.org" <chunkuang.hu@kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"krzk+dt@kernel.org" <krzk+dt@kernel.org>,
"Singo Chang (張興國)" <Singo.Chang@mediatek.com>,
"Nancy Lin (林欣螢)" <Nancy.Lin@mediatek.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"airlied@gmail.com" <airlied@gmail.com>,
"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
"Xavier Chang (張獻文)" <Xavier.Chang@mediatek.com>,
"Moudy Ho (何宗原)" <Moudy.Ho@mediatek.com>
Subject: Re: [PATCH v3 1/7] dt-bindings: mailbox: mediatek: Add MT8196 support for gce-mailbox
Date: Wed, 22 Jan 2025 02:34:55 +0000 [thread overview]
Message-ID: <cccfba4f0a55b4bb952616414158f5173317654b.camel@mediatek.com> (raw)
In-Reply-To: <CABb+yY3U9Ud5KNQ-4V9m327ZdumDPperRXvkaVV2RbKscSYH-Q@mail.gmail.com>
Hi Jassi,
Thanks for the reviews.
[snip]
> > > > 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..9e0700236033
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/mailbox/mediatek,mt8196-gce.h
From the previous discussion with Krzysztof:
https://lore.kernel.org/all/42def68200b28b5bea3cbf091907343976482132.camel@mediatek.com/
We'll drop this dt-binding header, ad move it to the dts folder.
> > > > @@ -0,0 +1,1415 @@
> > >
> > > 1415 ? 90% seem unnecessary.
> > >
> > > > +
> > > > +/* 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
> > > >
> > > Only need to HIGHEST maybe
> >
> > Or maybe we could just get rid of them and describe the valid
> > values
> > and ordering in the YAML part?
But actually, even if we describe this in the YAML, most of the GCE
users still won't know what is the meaning of each mbox-cells.
So I think if we can keep these define to make dts more readable.
What do you think?
> >
> > > > +
> > > > +/* 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
> > > > +
> > > Keep only that you use now or plan in the near future. But ok.
I think we can drop this in MT8196 because GCE hardware designer didn't
change these subsys id from the previous SoCs.
Almost all of the device register are over 0x3000_0000, so it's no
longer to use in MT8196.
> > >
> > > > +/* GCE-D hardware events */
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF0
> > > > 0
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF1
> > > > 1
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF2
> > > > 2
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF3
> > > > 3
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF4
> > > > 4
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF5
> > > > 5
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF6
> > > > 6
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF7
> > > > 7
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF8
> > > > 8
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF9
> > > > 9
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF10
> > > > 10
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF11
> > > > 11
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF12
> > > > 12
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF13
> > > > 13
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF14
> > > > 14
> > > > +#define
> > > > CMDQ_EVENT_DISP0_STREAM_SOF15
> > > > 15
> > > >
> > > you mean
> > > #define CMDQ_EVENT_DISP0_STREAM_SOF(n) n
Yes, it's the same meaning.
[snip]
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_WDMA0_TARGET_LINE_END_ENG_EVENT
> > > > 32
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_WDMA0_SW_RST_DONE_ENG_EVENT
> > > > 33
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_POSTMASK1_RST_DONE_ENG_EVENT
> > > > 34
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_POSTMASK0_RST_DONE_ENG_EVENT
> > > > 35
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_TIMEOUT_ENG_EVENT
> > > > 36
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT0
> > > > 37
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT1
> > > > 38
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT2
> > > > 39
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT3
> > > > 40
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT4
> > > > 41
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT5
> > > > 42
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT6
> > > > 43
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT7
> > > > 44
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT8
> > > > 45
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT9
> > > > 46
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT10
> > > > 47
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT11
> > > > 48
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT12
> > > > 49
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT13
> > > > 50
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT14
> > > > 51
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT15
> > > > 52
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MUTEX0_GET_RELEASE_ENG_EVENT
> > > > 53
> > > > +#define
> > > > CMDQ_EVENT_DISP0_DISP_MDP_RDMA0_SW_RST_DONE_ENG_EVENT
> > > > 54
> > > > +
> > > keep only the used ones and use
> >
> > This is the only publicly available table of the numbers. Having
> > the complete table somewhere would be nice. OOTH the numbers being
> > like IRQ or DRQ numbers, don't actually get used in the driver.
> > So maybe we could keep the full list but move it under the dts
> > directory?
> >
> Why introduce bloat in the kernel? We shouldn't be carrying what are
> basically 'addition' tables, not even 'multiplication' ;)
> The same knowledge can be represented concisely by the formula
> #define CMDQ_EVENT_DISP0_DISP_MUTEX0_REG_UPDATE_ENG_EVENT(n) (n +
> 37)
> especially for ~50 char defines
>
> thnx
I agree with you, I'll simplify the define with post-fixed numbers, and
move it to dts folder once the mt8196.dtsi with gce node is submitted.
Thanks for your suggestion.
Regards,
Jason-JH Lin
next prev parent reply other threads:[~2025-01-22 2:37 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-19 17:07 [PATCH v3 0/7] Add GCE support for MT8196 Jason-JH.Lin
2024-12-19 17:07 ` [PATCH v3 1/7] dt-bindings: mailbox: mediatek: Add MT8196 support for gce-mailbox Jason-JH.Lin
2024-12-27 8:11 ` Krzysztof Kozlowski
2024-12-30 2:25 ` Jason-JH Lin (林睿祥)
2024-12-27 8:13 ` Krzysztof Kozlowski
2024-12-30 9:23 ` Jason-JH Lin (林睿祥)
2025-01-03 17:04 ` Krzysztof Kozlowski
2025-01-07 2:42 ` Jason-JH Lin (林睿祥)
2025-01-07 6:16 ` Krzysztof Kozlowski
2025-01-07 7:26 ` Jason-JH Lin (林睿祥)
2025-01-18 21:24 ` Jassi Brar
2025-01-20 6:46 ` Chen-Yu Tsai
2025-01-20 16:56 ` Jassi Brar
2025-01-22 2:34 ` Jason-JH Lin (林睿祥) [this message]
2024-12-19 17:07 ` [PATCH v3 2/7] mailbox: mtk-cmdq: Add driver data to support for MT8196 Jason-JH.Lin
2024-12-19 17:07 ` [PATCH v3 3/7] soc: mediatek: mtk-cmdq: Add pa_base parsing for unsupported subsys ID hardware Jason-JH.Lin
2024-12-19 17:07 ` [PATCH v3 4/7] soc: mediatek: mtk-cmdq: Add mminfra_offset compatibility for DRAM address Jason-JH.Lin
2024-12-19 17:07 ` [PATCH v3 5/7] soc: mediatek: Add programming flow for unsupported subsys ID hardware Jason-JH.Lin
2024-12-19 17:07 ` [PATCH v3 6/7] drm/mediatek: " Jason-JH.Lin
2024-12-24 5:21 ` CK Hu (胡俊光)
2024-12-19 17:08 ` [PATCH v3 7/7] media: mediatek: mdp3: " Jason-JH.Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cccfba4f0a55b4bb952616414158f5173317654b.camel@mediatek.com \
--to=jason-jh.lin@mediatek.com \
--cc=Moudy.Ho@mediatek.com \
--cc=Nancy.Lin@mediatek.com \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=Singo.Chang@mediatek.com \
--cc=Xavier.Chang@mediatek.com \
--cc=airlied@gmail.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=chunkuang.hu@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jassisinghbrar@gmail.com \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=robh@kernel.org \
--cc=simona@ffwll.ch \
--cc=wenst@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).