Message-ID: <90b38dd6e093c89a7383814db786321cbd4533f6.camel@mediatek.com>
Date: Tue, 7 Jan 2025 02:42:15 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "krzk@...nel.org" <krzk@...nel.org>
CC: "robh@...nel.org" <robh@...nel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "simona@...ll.ch" <simona@...ll.ch>,
	Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
	Singo Chang (張興國) <Singo.Chang@...iatek.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "airlied@...il.com"
	<airlied@...il.com>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, Moudy Ho (何宗原)
	<Moudy.Ho@...iatek.com>, Xavier Chang (張獻文)
	<Xavier.Chang@...iatek.com>
Subject: Re: [PATCH v3 1/7] dt-bindings: mailbox: mediatek: Add MT8196 support
 for gce-mailbox

On Fri, 2025-01-03 at 18:04 +0100, Krzysztof Kozlowski wrote:

[snip]

> > 
> > Please forgive me for putting a lot of redundant message. I just
> > want
> > to provide as much detail as possible to help you determine if they
> > are
> > bindings. I appreciate your guidance and will make the necessary
> > adjustments.
> > 
> > 
> > I checked the clk header you accepted before:
> > https://lore.kernel.org/all/402ac5a2-334e-1843-0517-5ecf61f6a965@linaro.org/
> 
> With arguments like "someone, somewhere acked this, so I am allowed
> as
> well to send it" you enter tricky grounds.

Hmm...
I didn't mean that, I just want to find a correct reference.

> 
> > 
> > Please don't mind me to make a confirmation here because I can't
> > find
> > the documentation of the definition for binding header.
> > Do you mean all the header defined in include/dt-bindings/* should
> > be
> > used in a specific driver and the DTS in the same time?
> 
> Yes, otherwise how is it abstraction?

OK, then I think it's not abstract, because we just use it in the DTS.

> 
> There are numerous exceptions of course when binding binds different
> pieces of software/firmware. Is that the case? Nothing in commit msg
> explained this.

No, I think it is not the case. I'll drop it.

> 
> > 
> > Take the `#define CLK_TOP_AXI` and `#define CLK_TOP_VPP` in
> > mediatek,mt8188-clk.h for example:
> > 
> > `CLK_TOP_AXI` is used in the drivers/clk/mediatek/clk-mt8188-
> > topckgen.c
> > but not in arch/arm64/boot/dts/mediatek/mt8188.dtsi:
> > ```
> >   #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > 
> >   ...
> > 
> >   static const struct mtk_mux top_mtk_muxes[] = {
> >       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_AXI, "top_axi",
> > axi_parents,
> >                                  0x020, 0x024, 0x028, 0, 4, 7,
> > 0x04,
> >   ...
> > 
> >       MUX_GATE_CLR_SET_UPD(CLK_TOP_VPP, "top_vpp",
> >                            vpp_parents, 0x02C, 0x030, 0x034, 0, 4,
> > 7,
> >   ...
> > ```
> > 
> > and `CLK_TOP_VPP` is used in the both clk-mt8188-topckgen.c and
> > mt8188.dtsi:
> > ```
> >   power-domain@...188_POWER_DOMAIN_VPPSYS0 {
> >       reg = <MT8188_POWER_DOMAIN_VPPSYS0>;
> >       clocks = <&topckgen CLK_TOP_VPP>,
> >                <&topckgen CLK_TOP_CAM>,
> > ...
> > ```
> > 
> > But it seems that both of `CLK_TOP_AXI` and `CLK_TOP_VPP` are
> > regarded
> > as binding headers.
> 
> I don't get the comparisons. Both constants represent abstraction, so
> they are correct.
> 

OK, I just want to make sure that it is not a `must` to use the binding
header in driver code and DTS at the same time.

> 
> > 
> > 
> > From the previous description of the example you gave me:
> > Bindings are imaginary numbers starting from 0 or 1 which are used
> > between drivers and DTS, serving as abstraction layer (or
> > abstraction
> > values) between these two.
> 
> Exactly, what to say more?
> 

So the key point is abstract.

> > 
> > As I understand, each clock definition corresponds to the clock CG
> > settings provided to different hardware, and each hardware driver
> > can
> > control its own clock CG through the CCF to control their CG in
> > clock
> > driver. So they can be an abstraction values between driver and
> > DTS.
> > 
> > Similarly, the GCE subsys ID and GCE event ID correspond to symbols
> > used by GCE to control various hardware, and each hardware driver
> > can
> > use these IDs to generate commands buffer for GCE through the API
> > provided by the GCE driver and achieve the desired control over
> > their
> > hardware.
> 
> So are they abstract or not? Then use some different values, really
> abstract.
> 
> I brought you definition: abstract. You now cited it. But last
> paragraph
> entirely skips this point.
> 

OK, so maybe I should use a platform-specific driver code to use it as
an abstract number for the real hardware settings.
e,g.

mt8195-cmdq-platform.c:
```
thread_prio[CMDQ_THR_PRIO_LOWEST] = 0,
thread_prio[CMDQ_THR_PRIO_1] = 0x1,
...
thread_prio[CMDQ_THR_PRIO_HEIGHEST] = 0x7,
```

and

mt8188-cmdq-platform.c:
```
thread_prio[CMDQ_THR_PRIO_LOWEST] = BIT(0),
thread_prio[CMDQ_THR_PRIO_1] = BIT(1),
...
thread_prio[CMDQ_THR_PRIO_HIGHEST] = BIT(7),
```

But I think it's not necessary in its case.
So I'll drop this dt-bindings header.

> 
> > 
> > I guess the difference is that the clock driver has a platform-
> > specific
> > clock table to store these binding headers, while the GCE driver
> > does
> > not have a platform-specific thread priority table, subsys ID
> > table,
> > and event ID table. Instead, the GCE client drivers can directly
> > obtain
> > their respective hardware settings from the DTS.
> > 
> > On the other hand, definitions like CLK_TOP_MAINPLL_D3,
> > CLK_TOP_MAINPLL_D4, etc., correspond to different clock frequency
> > divider levels, and the CMDQ_THR_PRIO_X for GCE thread priority
> > also
> > corresponds to different priority levels for GCE threads.
> > Therefore, I
> > am not quite sure why GCE thread priority cannot be considered a
> > binding when it is also a symbol number for a hardware level
> > setting.
> 
> Well, maybe nothing here is a binding. I took one thing to inspect. I
> did not inspect the rest. How does it help your case?
> 

Yes, considering the abstraction, there are no bindings here.

> 
> > 
> > 
> > If the condition for becoming a binding header is that it `must` be
> > used by a specific driver, such as a platform-specific table, then
> > I
> 
> No, "used by the driver" is indication that you use it as
> abstraction.
> 

OK, I got it.

> > will remove the entire GCE dt-binding header. Because the current
> > usage
> > of these definitions is that each GCE client drivers can directly
> > store
> > these GCE definitions through the DTS, just like IRQ IDs, and
> > without
> > the need for an additional table defined by the GCE driver.
> 
> Do you store IRQ IDs as binding constants in binding headers? No.
> Why?
> 
> Before proceeding with this header further, please answer to above -
> why
> interrupt numbers, MMIO addresses and some other values appearing in
> DTS
> are not used like "binding headers".

Because interrupt numbers and MMIO addresses are the real numbers of
hardware settings. Their driver can get them directly from their device
node in DTS. They are actual number to be set into their hardware, so
they don't need to be translated in their platform drivers.

So I think all the definitions in the `mediatek,mt8196-gce.h` are the
same case. They are actual hardware numbers for GCE hardware to use.
It should be drop from the include/dt-bindings/*.


BTW, to make these numbers more readable in DTS, can I move
`include/dt-bindings/mailbox/mediatek,mt8196-gce.h` into
`arch/arm64/boot/dts/mediatek/mt8196-gce.h`?

Just like the `arch/arm64/boot/dts/mediatek/mt8167-pinfunc.h`.

Regards,
Jason-JH.Lin

> 
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists