On 30/12/2024 10:23, Jason-JH Lin (林睿祥) wrote:
> 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@xxxxxxxxxx/
With arguments like "someone, somewhere acked this, so I am allowed as
well to send it" you enter tricky grounds.
>
> 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?
There are numerous exceptions of course when binding binds different
pieces of software/firmware. Is that the case? Nothing in commit msg
explained this.
>
> 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.
I don't get the comparisons. Both constants represent abstraction, so
they are correct.
>
>
> 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?
>
> 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.
>
> 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?
>
>
> 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.
> 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".
Best regards,
Krzysztof
