Re: [PATCH v3 1/7] dt-bindings: mailbox: mediatek: Add MT8196 support for gce-mailbox
From: Jason-JH Lin (林睿祥)
Date: Tue Jan 21 2025 - 21:41:25 EST
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@xxxxxxxxxxxx/
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