Re: [PATCH v2 1/8] dt-bindings: mailbox: mediatek: Add GCE header file for MT8196

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/12/2024 04:05, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Wed, 2024-12-11 at 10:37 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On Wed, Dec 11, 2024 at 11:22:49AM +0800, Jason-JH.Lin wrote:
>>> Add the Global Command Engine (GCE) header file to define the GCE
>>> thread priority, GCE subsys ID and GCE events for MT8196.
>>
>> This we see from the diff. What we do not see is why priority is a
>> binding. Looking briefly at existing code: it is not a binding, there
>> is
>> no driver user.
>>
> 
> This priority value is used to configure the priority level for each
> GCE hardware thread, so it is a necessary hardware attribute.

I did not say these are not "hardware". I said these are not bindings.
Bring arguments why these are bindings.

> 
> It's hard to find where the priority is used in existing driver code
> because we parsed it from DTS.

So not a binding.

> 
> It is used in all mediaTeks' DTS using the GCE.
> For example, in mt8195.dts:
> 
> vdosys0: syscon@1c01a000 {
>     compatible = "mediatek,mt8195-vdosys0", "mediatek,mt8195-mmsys",
> "syscon";
>     reg = <0 0x1c01a000 0 0x1000>;
>     mboxes = <&gce0 0 CMDQ_THR_PRIO_4>;
>     #clock-cells = <1>;
>     mediatek,gce-client-reg = <&gce0 SUBSYS_1c01XXXX 0xa000 0x1000>;
> }
> 
> CMDQ driver(mtk-cmdq-mailbox.c) will get the args parsed from mboxes
> property in cmdq_xlate() and then it will store CMDQ_THR_PRIO_4 to the
> specific thread structure. 

So not a binding.

> The user of CMDQ driver will send command to CMDQ driver by 
> cmdq_mbox_send_data(), and this priority setting will be configured to
> GCE hardware thread.

And other things there are the same, we do not talk only about this one
thing. I asked last time to drop which is not a binding.


...

>>> +
>>> +/*
>>> + * GCE General Purpose Register (GPR) support
>>> + * Leave note for scenario usage here
>>> + */
>>> +/* GCE: write mask */
>>
>> That's a definite no-go. Register masks are not bindings.
>>
> 
> I'm sorry to the confusion.
> 
> These defines are the index of GCE General Purpose Register for
> generating instructions, they are not register masks.

Index of register is also sounding like register.

> 
> The comment "/* GCE: write mask */" is briefly describe that the usage
> of GCE_GPR_R0 and GCE_GPR_R01 is used to store the register mask when
> GCE executing the WRITE instruction. And it can also store the register
> mask of POLL and READ instruction.
> 
> I will add more words to make this comment clearer, like this:
> /*GCE: store the mask of instruction */

Not sure, because I feel you just avoid doing what is right and keep
pushing your own narrative. Where is it used in the driver?

I just looked for "GCE_GPR_R00" - no usage at all. So not a binding.

Best regards,
Krzysztof