Re: [PATCH v3 1/7] dt-bindings: mailbox: mediatek: Add MT8196 support for gce-mailbox

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

Hi Krzysztof,

On Fri, 2024-12-27 at 09:13 +0100, Krzysztof Kozlowski wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Dec 20, 2024 at 01:07:54AM +0800, Jason-JH.Lin wrote:
> >    2) GCE Subsys ID:
> >    - Defined in the header file: `#define SUBSYS_1c00XXXX 3`
> >    - Used in the Device Tree:
> >       `mediatek,gce-client-reg = <&gce SUBSYS_1c00XXXX 0x0000
> > 0x1000>;`
> >    - Parsed and used in the driver to configure subsys ID:
> >      ```c
> >      int cmdq_dev_get_client_reg(struct device *dev,
> >                                struct cmdq_client_reg *client_reg,
> >                                int idx)
> >      {
> >       client_reg->subsys = (u8)spec.args[0];
> >       client_reg->offset = (u16)spec.args[1];
> >      }
> >      // GCE write the value to the register 0x1c000000 + 0x0000 +
> > offset
> >      cmdq_pkt_write(cmdq_handle, client_reg->subsys,
> >                   client_reg->offset + offset, value);
> 
> This is a proof that SUBSYS_1300XXXX is not a binding. Your driver
> does
> not use it.
> 
> Drop all such things which are not used by drivers or explain why
> they
> are needed to be in the binding - what do they bind.
> 
> I asked for this already, for exactly the same thing.
> 
> 
> I did not check the rest, so next time I will choose any other random
> define and if I do not find it explained nor used, I will question
> it.
> Because you tend to apply pieces of review instead of really change
> your
> code.

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/

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?

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@MT8188_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.


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.

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.

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.


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
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.


I am not unwilling to change the code, but I hope to first understand
what qualifies as a binding header.
This way, I can confidently remove the MT8196 GCE binding header,
and other developers will also know that the GCE binding header for the
previous MTXXXX is not needed.

I sincerely hope you can guide me to meet your expectations and
standards.

Regards,
Jason-JH.Lin

> 
> >      ```
> 
> Best regards,
> Krzysztof
>